Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: footer should not fix to the bottom #70

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: footer should not fix to the bottom #70

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a85bed2
fix: footer should not fix to the bottom
CIPHERTron Mar 23, 2022
411053b
stick footer at bottom when content is less
CIPHERTron Mar 29, 2022
606036f
Merge branch 'main' into fix-footer
krancour Mar 30, 2022
38132b2
fix: revert changes made to footer
CIPHERTron Apr 1, 2022
64a8662
Merge branch 'fix-footer' of https://github.com/CIPHERTron/brigade-da…
CIPHERTron Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/App.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,16 @@ $text: #647b83;
$work: "work_sansregular", "Work Sans", "Helvetica Neue", Helvetica, Arial,
Geneva, sans-serif;

html {
height: 100%;
}

body {
font-family: $work !important;
padding-bottom: 80px;
position: relative;
margin: 0;
padding-bottom: 5rem;
min-height: 100%;
}

a:not(.nav-link) {
Expand Down Expand Up @@ -74,3 +81,16 @@ a:not(.nav-link) {
color: $text !important;
}
}

footer {
position: absolute;
right: 0;
bottom: 0;
left: 0;
background-color: #212529;
color: #6c757d;
min-height: 50px;
padding: 0 5%;
display: flex;
align-items: center;
}
8 changes: 1 addition & 7 deletions src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,7 @@ export default class App extends React.Component<unknown, AppState> {
</Container>
</main>
<footer>
<Navbar bg="dark" variant="dark" expand="lg" fixed="bottom">
<Container>
<span className="text-muted">
&copy; 2022 The Brigade Authors
</span>
</Container>
</Navbar>
Copy link
Contributor

@DhairyaBahl DhairyaBahl Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kindly tell me why you changed these lines to a single line ? What issue you were facing over here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier, the footer was using the Navbar component from react-bootstrap which is actually semantically incorrect (i.e. using a Navbar component for the footer). Besides, the footer didn't have much content. Thus, I removed all the code and used only a simple footer element :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicee @CIPHERTron ! Great Work !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things here.

I'm not sure where the idea comes from that using a navbar in the footer is semantically incorrect. Bootstrap's own official examples include numerous footers that contain navbars:

https://getbootstrap.com/docs/5.1/examples/footers/

Beyond that, a navbar was used in the footer to account for the high probability of the footer being expanded to include a variety of different links.

All of that having been said, I don't really want to waste a lot of time arguing over whether the navbar belongs or not...

The container element certainly should not go away because it affects the appearance of the page. As in the case of the top navbar and the main body of the page, the container element was used in the footer to constrain content closer to the center of larger viewports.

If you look at the deployment preview in Netlify, you will see this has now changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krancour Oh okay, I'll revert the changes and add back the container and Navbar component.

<span>&copy; 2022 The Brigade Authors</span>
</footer>
</Fragment>
)
Expand Down