-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add new Navigator components and use them in the global styles sidebar #34904
Conversation
Size Change: +584 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
a66299e
to
6dc5e41
Compare
0731c22
to
b2ef029
Compare
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.
Nice work on this PR @youknowriad. Things are working well for me 👍 I left some questions related mostly to our matching mechanism. In general, I like this approach it is simple and I think it seems like a good fit for our needs. I guess we can merge this PR.
@@ -8,14 +8,28 @@ import { map } from 'lodash'; | |||
*/ | |||
import { | |||
Button, | |||
__experimentalNavigation as Navigation, |
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.
What's the plan regarding these components? Are they going to be refactored and use __experimentalNavigator inside?
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.
That's my hope yes. (I'd appreciate some help there with existing usage :P )
) ) } | ||
</NavigationMenu> | ||
</NavigatorScreen> | ||
|
||
{ map( blocks, ( block, name ) => ( |
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.
For blocks that don't support any global styles panel, we should not render a menu for them. I think we regressed on #34885.
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.
We can leave this separate I think to avoid having a diff unrelated with the change of the navigation components.
I would prefer to wait before merging until we have a clear understanding of the approach we're going to take with the navigation components if that's possible |
8de9873
to
e2028bc
Compare
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.
Thank you for iterating on this new set of components, @youknowriad ! Overall the code looks much cleaner, and it decouples the UI from the functionality.
I just wanted to mention that in both the GLobal Styles sidebar and the Storybook example, I can spot a horizontal scrollbar while two screens are animating at the same time. Here's a video (I slowed down the animation times to show this better):
navigator-scrollbars.mp4
Apart from the scrollbar, in my mind the next steps would be:
- convert the
Navigator
components to TypeScript - add some basic a11y features:
- focus management: when clicking on a link, the new
Screen
should receive focus - RTL support: on RTL languages, we should swap the direction of the animation
- reduced motion: we should disable the animations completely in case the user has expressed a preference for reduced motion
- focus management: when clicking on a link, the new
Thanks for the review Marco.
I implemented both of these.
Good catch, it's not really clear how to solve and keep the same animation, adding a "overflow: hidden" could be a solution about I'm worried about side effects (hidden popovers potentially)
😬 I'm not yet familiar with that yet but I can try
I implemented a naive version of this. This might require some iterations or input from others. |
Actually the typescript conversion here has proven a bit challenging for me, I'd love to see someone else do it (and learn from it) either here or separately. |
Thank you for working on the RTL, reduced motion and focus management — they all seem to work well for me and I think they make for a good initial set of a11y improvements. We should probably add a changelog entry to the I suggest that we merge this PR, and work on a few follow-ups:
What do folks think? |
@ciampo This plan makes sense to me. |
62ff572
to
9060a47
Compare
borrows from #32923 and refactors the navigation in global styles sidebar from #34885 to use the new Navigator components. This is more flexible in terms of design though it requires a bit more code.