Refactor the preferences modal to use the new Navigator components #35142
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
youknowriad
added
[Type] Code Quality
youknowriad
commented
Sep 27, 2021
youknowriad
commented
Sep 27, 2021
|
Size Change: +216 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Sep 27, 2021
There was a problem hiding this comment.
I really like how this is looking!
Here's a comparison of what's in production vs the version from this PR (I've added the green lines to show the icon alignment):
| Production | This PR |
|---|---|
 |
 |
Overall, I believe there's a couple of adjustments to be made:
- The chevron icons need to be better aligned with the cross icon (I've added green guidelines in the screenshots to showcase this better)
- The text used for the menu items is smaller and darker compared to what we currently have in production
It would probably be a good idea for @jasmussen to have a quick look too
I'm not sure if it's necessary to align with what we have in trunk, I think the best solution here would be to design it in the most "common" way. Meaning, I shouldn't have to customize the look and feel and spacing in the sidebar itself, but rather use the built-in styles that come from the components. So if there are customization to be made, we should make sure that they are generic enough to be applied to the root components themselves. (still in the same mindset that as a consumer I should compose components and not customize them) |
That makes sense — we should then make sure that the default styles provided by the components match our needs, or if they need any tweaking. Also, it would probably be a good idea to wrap the text inside a |
6 tasks
ntsekouras
reviewed
Sep 28, 2021
| <NavigationButton path="/"> | ||
| { __( 'Back' ) } | ||
| </NavigationButton> | ||
| <h2>{ section.tabLabel }</h2> |
There was a problem hiding this comment.
Shouldn't we add a css class here and style better as in trunk?
There was a problem hiding this comment.
I personally don't feel like the styles in trunk (the ones inherited from the menus UI) work well here. I prefer the default h2 styles, that said, I'm happy to update if y'all feel otherwise.
What do you mean here? If I understand correctly you're saying that if I want to style things a bit differently, I should be able to pass something like |
Yes, that's exactly what I mean. Ideally we shouldn't have to customize things other than with using the regular props offered by the UI components. This has several advantages:
|
ntsekouras
reviewed
Sep 28, 2021
ciampo
reviewed
Sep 28, 2021
youknowriad
force-pushed
the
update/refactor-preferences-modal-navigator
branch
from
c9ea056
to
ea3aceb
Compare
3 tasks
6 tasks
This was referenced Oct 4, 2021
4 tasks
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.
Follow-up to #34904
Now that the new Navigator components are merged, let's try to use them in all existing places in the Gutenberg codebase. We have two usage of the old components:
In this PR, I'm starting with the simplest one: the preferences to see if our current UI components can compose well enough to achieve a similar result. The result right now is a bit different than trunk but it's close enough.