feat: add top-level nav styles#883
Merged
Merged
Conversation
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.
b7b21f9 to
74f26e7
Compare
greyscaled
reviewed
Apr 6, 2022
Contributor
greyscaled
left a comment
There was a problem hiding this comment.
@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!
greyscaled
reviewed
Apr 6, 2022
| <div className={styles.fullWidth} /> | ||
| <div className={styles.fixed}>{user && <UserDropdown user={user} onSignOut={onSignOut} />}</div> | ||
| </div> | ||
| </nav> |
Contributor
There was a problem hiding this comment.
praise: for fixing the semantics of this HTML
Member
Author
|
Oh oops thanks for the heads up! |
presleyp
reviewed
Apr 6, 2022
| }, | ||
|
|
||
| // NavLink adds this class when the current route matches. | ||
| "&.active": { |
presleyp
approved these changes
Apr 6, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.