fix: footer should not fix to the bottom #70
Conversation
Signed-off-by: CIPHERTron <[email protected]>
✅ Deploy Preview for brigade-dashboard ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
@CIPHERTron this is doing what I was trying to avoid-- the footer isn't pushed all the way to the bottom of the viewport unless there's a certain amount of content. This looks weird:
|
Ah okay, I had not thought of this particular case haha |
Heyy @krancour how can I access this |
|
@CIPHERTron if you're running the dashboard locally, set the env var You can also look at the deployment preview created in response to this PR on Netlify. Just click |
Signed-off-by: CIPHERTron <[email protected]>
Screen.Recording.2022-03-29.at.11.57.28.AM.mov |
|
Heyy @krancour I've fixed the above-mentioned issue. Now, if the screen has less content then the footer will be at the bottom of the screen but not fixed at the bottom. In other cases, it'll be at the complete bottom. |
There was a problem hiding this comment.
Rest of the changes are good to go acc to me @krancour and I believe that all the changes in CSS are apt. @CIPHERTron kindly take the pull from upstream.
Rest is Ok from my side.
src/App.tsx
Outdated
| <Navbar bg="dark" variant="dark" expand="lg" fixed="bottom"> | ||
| <Container> | ||
| <span className="text-muted"> | ||
| © 2022 The Brigade Authors | ||
| </span> | ||
| </Container> | ||
| </Navbar> |
There was a problem hiding this comment.
Kindly tell me why you changed these lines to a single line ? What issue you were facing over here ?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Nicee @CIPHERTron ! Great Work !
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@krancour Oh okay, I'll revert the changes and add back the container and Navbar component.
|
@krancour I guess everything is good to go from my side. |
Signed-off-by: CIPHERTron <[email protected]>
|
@krancour I've reverted the changes made to footer. Lmk if there're any changes that needs to be done. |
|
I appreciate the effort that's gone into this. I am not a CSS expert by any stretch of the imagination (I'm a backend guy), and when I got this project started, I tried really hard to keep the CSS as minimal and simple as possible. Minimal and simple == easy to understand -- not just for myself, but for everyone. So those are ideals I'd like us to try to stick to as we move forward. Absolute positioning is a red flag that we're interrupting the normal flow of the elements, and that's something that can almost instantly confuse a non-CSS expert like myself. So just now I've spent some time researching possible alternatives and flexboxes look really attractive. (Just to prove how non-expert I am with CSS, I don't think flexboxes existed last time I did any significant amount of CSS -- 10+ years ago.) From what I've read (this was the most helpful resource I found), flexboxes would intuitively preserve the normal flow of elements and allows us to specify if, or to what extent, elements are permitted to grow or shrink. This seems to me to be exactly what we want. We want:
With a little experimentation, this small bit of SCSS seems to do the trick: html, body, #root {
height: 100%;
}
body {
font-family: $work !important;
}
#root {
display: flex;
flex-direction: column;
}
header, footer {
flex: 0 0 auto;
}
main {
flex: 1 1 auto;
padding: 80px;
}So now let me ask you guys, @CIPHERTron and @DhairyaBahl, as people who probably know CSS better than myself, is this approach sensible? Or are there reasons that using absolute positioning is superior? |
|
@krancour Sorry for delayed reply, Actually my college exams are going on, that's why my schedule is a bit tight till this saturday. Talking about CSS, even I try to stay away from absolute positioning as much as possible. It do makes a lot of things easy but it may break the website's responsiveness if not handled/done correctly. Also +1 for the point you wrote about the code being difficult to understand for beginner. Talking about what I would do for this particular use case, there is no need to use flex over here in column to arrange header main and footer. I would just set a minimum height for the main and it would automatically expand vertically when needed.🙂 |
If that works, it sounds like the simplest of all options. Let's try that. |
Signed-off-by: CIPHERTron [email protected]
fixes #63