brigadecore / brigade-dashboard

Apache License 2.0
4 stars 5 forks source link

fix: footer should not fix to the bottom #70

Open CIPHERTron opened 2 years ago

CIPHERTron commented 2 years ago

Signed-off-by: CIPHERTron pritish.samal918@gmail.com


fixes #63

netlify[bot] commented 2 years ago

Deploy Preview for brigade-dashboard ready!

Name Link
Latest commit 64a86620515aae88f5a71c101c530bf2230af024
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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

https://user-images.githubusercontent.com/56754747/160547419-7cc3ffff-6bbf-4c04-a918-aae64ae38890.mov

CIPHERTron commented 2 years ago

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.

DhairyaBahl commented 2 years ago

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

CIPHERTron commented 2 years ago

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

krancour commented 2 years ago

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?

DhairyaBahl commented 2 years ago

@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 commented 2 years ago

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.