- Notifications
You must be signed in to change notification settings - Fork928
feat: add top-level nav styles#883
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedApr 6, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #883 +/- ##==========================================- Coverage 65.80% 65.53% -0.28%========================================== Files 123 216 +93 Lines 12570 13734 +1164 Branches 0 103 +103 ==========================================+ Hits 8272 9000 +728- Misses 3406 3810 +404- Partials 892 924 +32
Continue to review full report at Codecov.
|
Partially addresses#701.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@code-asher - might want to update PR title/desc, ticket and impl to match#880 ---> seems it came in recently from external feedback right after this was posted, sorry that it will require adjustments here!
| <div className={styles.fullWidth} /> | ||
| <div className={styles.fixed}>{user && <UserDropdown user={user} onSignOut={onSignOut} />}</div> | ||
| </div> | ||
| </nav> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
praise: for fixing the semantics of this HTML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yay!
Oh oops thanks for the heads up! |
| }, | ||
| // NavLink adds this class when the current route matches. | ||
| "&.active": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
chef's kiss
Uh oh!
There was an error while loading.Please reload this page.
Partially addresses#701.
Pretty much just adds the styling for top-level nav items. For now it contains one link (projects, soon to be templates).
I avoided the nested navbar entry component since it looked fairly clean to use ListItem and NavLink at the top to me. NavLink also handles the active link (looks like we were doing that manually before) and I left out the resize handlers.
If we port over the route config or something similar we can add a loop where the projects/template link is.
Originally I had the organizations dropdown in there too but I stashed that for now since I saw there was some overlap in Presley's tickets so I thought I might be stepping on toes with the bordered menu (the row, label, etc, I wanted to flatten things a bit as well), plus it will be easier to implement that dropdown once we actually have some organization stuff to link it to anyway (but let me know if it would be better for me to port that over; I am perfectly happy to do so).
The projects/templates link does feel a little redundant since the logo goes to the same place but I suppose it does have the advantage of explicitly being called "projects" so it is more discoverable.
As an aside, does anyone know why
<ListItem button>is adivinstead ofli? Seems like a bug with MUI potentially. Ideally I would not usebuttonat all but idk how else to get the ripple effect on click. Ideally it would be supported onNavLink.