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.

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

Conversation

CIPHERTron
Copy link
Contributor

Signed-off-by: CIPHERTron [email protected]


fixes #63

@netlify
Copy link

netlify bot commented Mar 23, 2022

Deploy Preview for brigade-dashboard ready!

Name Link
🔨 Latest commit 64a8662
🔍 Latest deploy log https://app.netlify.com/sites/brigade-dashboard/deploys/6246a2c3778a4f000914cefb
😎 Deploy Preview https://deploy-preview-70--brigade-dashboard.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@krancour
Copy link
Contributor

@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:

Screen Shot 2022-03-23 at 8 18 51 AM

@CIPHERTron
Copy link
Contributor Author

@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:

Screen Shot 2022-03-23 at 8 18 51 AM

Ah okay, I had not thought of this particular case haha
Let me try to fix this too 💪🏻

@CIPHERTron
Copy link
Contributor Author

@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:

Heyy @krancour how can I access this /users route?
Right now I'm unable to do so :(

@krancour
Copy link
Contributor

@CIPHERTron if you're running the dashboard locally, set the env var REACT_APP_BRIGADE_API_ADDRESS to https://api.brigade2.io. That's Brigade's own Brigade instance and we're permitting global read-only access.

You can also look at the deployment preview created in response to this PR on Netlify. Just click details next to the netlify/brigade-dashboard/deploy-preview check.

@CIPHERTron
Copy link
Contributor Author

Screen.Recording.2022-03-29.at.11.57.28.AM.mov

@CIPHERTron
Copy link
Contributor Author

CIPHERTron commented Mar 29, 2022

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.

Copy link
Contributor

@DhairyaBahl DhairyaBahl left a comment

Choose a reason for hiding this comment

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

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
Comment on lines 99 to 105
<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

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.

@DhairyaBahl
Copy link
Contributor

@krancour I guess everything is good to go from my side.

@CIPHERTron
Copy link
Contributor Author

@krancour I've reverted the changes made to footer. Lmk if there're any changes that needs to be done.

@krancour
Copy link
Contributor

krancour commented Apr 1, 2022

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:

  • A header that doesn't grow or shrink
  • A main content area that uses as much space as it needs or grows to fill remaining space if there is any
  • A footer that doesn't grow or shrink

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?

@DhairyaBahl
Copy link
Contributor

@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.🙂

@krancour
Copy link
Contributor

krancour commented Apr 6, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proposal: footer should not stick to the bottom
3 participants