Refactor SideNavigation #2872
Open
Refactor SideNavigation #2872
+264
−202
Conversation
This file contains 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
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: -683 B (-0.1%) Total Size: 674 kB
ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #2872 +/- ##
==========================================
+ Coverage 88.12% 88.17% +0.05%
==========================================
Files 228 228
Lines 13188 13177 -11
Branches 1809 1812 +3
==========================================
- Hits 11622 11619 -3
+ Misses 1512 1504 -8
Partials 54 54
|
connor-baer
approved these changes
Jan 18, 2025
Comment on lines
+5
to
7
| .base > button { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
The mobile SideNavigation's close button is left-aligned to match the location of the hamburger button in the TopNavigation that likely opens it.
I wonder if it'd be better to right-align it for consistency with the other dialog components. When a user opens the SideNavigation, they'll likely scann the links and then look for the close button in the familiar top-right corner. WDYT?
There was a problem hiding this comment.
Although I am always for consistency, I like having the close button in the same spot as the hamburger. Its a pattern we had for a long time now, widely used in most websites, and since the button only concerns mobile, it makes it easier for users to tap the same spot to close than to stretch their thumbs all the way across the screen to the other side :lazy:
There was a problem hiding this comment.
it makes it easier for users to tap the same spot to close than to stretch their thumbs all the way across the screen to the other side :lazy:
That's exactly my point though: having the close button on the left side means users have to stretch further to reach the close button. They're not going to keep their thumb hovering at the same spot over the screen as that would obscure the navigation menu.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Addresses DSYS-905
Purpose
Replace ReactModal with the Dialog component in the SideNavigation and remove the usages of the
useMobileNavigationhook and theModalProviderin the SideNavigation.Definition of done