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

Don't add 200px left margin for toolbar on sign-in page. #404

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

mrabbitt
Copy link
Contributor

@mrabbitt mrabbitt commented Sep 4, 2024

Fix to only add the margin-left: 200px needed for the navigation side bar on pages other than the sign-in page.

Fixes #403.

// TODO?: Are there better ways to achieve this?
//Sets purple background for student profile and instructions routes
const isPurpleBg =
router.asPath.includes("/studentprofile") ||
router.asPath.includes("/instructions");
const hasNavBar = "authenticated" == status;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replicates here the logic in the NavBar component to decide if the NavBar should be displayed:

{status === "authenticated" && (

Copy link
Contributor

@KCCPMG KCCPMG left a comment

Choose a reason for hiding this comment

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

Does what it says, looks good to me. I'm wondering if there's an original sin with this whole issue in that the Drawer component that the Navbar uses should be able to take care of the spacing itself without having to add a margin to its sibling, but that's a more complicated issue that someone else could look into later. In the meantime, I'm happy to approve this and to get all my future sessions start off on a better foot.

@mrabbitt
Copy link
Contributor Author

mrabbitt commented Sep 9, 2024

Thanks, @KCCPMG !

I'm wondering if there's an original sin with this whole issue in that the Drawer component that the Navbar uses should be able to take care of the spacing itself without having to add a margin to its sibling, but that's a more complicated issue that someone else could look into later.

Agreed.

It might also be worth looking into using per-page layouts to have different layouts for different pages. Right now, it seems like the sign-in page is the only exception, so that's probably overkill. If in the future there's a need for more pages that either don't have the nav bar or require other layouts, it may be worth it.

For the moment though, the change in this PR seemed to me to be the least disruptive improvement. I'll merge it.

@mrabbitt mrabbitt merged commit 542995f into main Sep 9, 2024
3 checks passed
@mrabbitt mrabbitt deleted the bugfix/sign-in-page-margin branch September 9, 2024 18:01
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.

Sign-in page layout not centered in wide desktop layout
2 participants