Preferences modal redesign #28329
Preferences modal redesign #28329
Conversation
|
Size Change: +80.9 kB (+6%) 🔍 Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Really nice and simple PR that takes a giant ticket and gets it going. Well done!
When opening, focus goes from the modal, to the close button, to the tabs on the left. The tabs on the left are navigated with the arrow keys. All good. But the tab needs a focus style:
I'd recommend just using the same standard focus style we use everywhere else:
I don't recall what control we have of the width of the modal, but I'd love to see it be just a teensy bit wider than this by default.
It doesn't feel like we have established a well thought out hierarchy of heading sizes. "Preferences" is a H1, "Choose your own experience" is a H2. The order is correct, but there's something about the font sizes that looks not as considered as it could be. This is perhaps best solved as a separate issue, but it's something that would benefit this effort. We don't actually use this gray active style anywhere else:
We sort of will, when #27331 gets a bit further (see toggled color swatch). But otherwise the most used pattern is to have this blue indicator:
I don't necessarily think that blue indicator is the best to use here either. Another problem is that "General" doesn't align to "Preferences", because of the big box padding. I tried a quick stab at an alternative, but that's also not great:
I think we can probably go with the gray background for now, and we can explore if/when we get a better pattern. But let's add a 2px border radius to the gray square, and make the non-active menu items be normal font weight:
Finally, there's a different spacing between Title/Helptext and switch on this screen:
Compared to this screen:
That's a lot of small things, but it should be quick to address! Let me know if I can help. |
| width: 400px; | ||
| padding-left: $grid-unit-30; | ||
| } | ||
| } |
There was a problem hiding this comment.
It feels like there's too much CSS needed for this. Were the base components styles (TabPanel and Navigation) not enough to achieve the desired result? Do we want to move some of these styles to the generic component or are these specific to the Preferences modal? Do we need the styles to be different from the original ones that come from the UI components?
There was a problem hiding this comment.
Unfortunately there will be more css changes :) - I wanted to get the first feedback. Navigation component was built for FSE with a dark theme so it's not fitting really well here. But since we're proceeding with this approach I'll explore the best place to put/change styles.
There was a problem hiding this comment.
Navigation component was built for FSE with a dark theme so it's not fitting really well here.
Yeah, for instance this makes me wonder if the Navigation component need to have dark styles :). Ideally, it just works no matter its container background, alternatively it could support both with a prop. isLight/isDark
There was a problem hiding this comment.
That's what I'm thinking as well!
There was a problem hiding this comment.
I think @ItsJonQ already had a light / dark version of this, which is going to be used (as light) in the global styles case.
There was a problem hiding this comment.
For context, light/dark mode is built into the Navigator component from the new component system.
The one we currently have in @wordpress/components isn't using that yet.
There was a problem hiding this comment.
Regarding the css in Tabs, I think it's okay to leave it here, based on Joen's comments:
We don't actually use this gray active style anywhere else
I think we can probably go with the gray background for now, and we can explore if/when we get a better pattern.
Regarding the Navigation css, I'm not sure how to properly integrate it in the component with @emotion (css-in-js).
Help with that and a sanity check on css rules would be really appreciated! @jasmussen, @ItsJonQ
|
Thanks for reviewing @jasmussen and @youknowriad! I think the approach is validated |
8077e67 to
c28a7e0
Compare
|
Haii! I took a quick look. Micro adjustments adjust (suggestions by @jasmussen ), an issue I've spotted would be the Modal header disappearing when scrolling down a long list:
From what I can see, it looks like there's a structural issue with the Modal setup. The entire Modal frame is scrollable, and the header/content are rendered inside. (with I may be wrong, but this feels like a deeper issue with the This aside. To achieve a more comfortable width (as @jasmussen had suggested) we could add the following CSS to width: calc(100% - 32px);
max-width: 920px;Of course, adjust the numbers as needed. |
|
Hey @ItsJonQ - thanks for checking!
I noticed that too as I struggle to make the |
|
Alright, I pushed some polish to use inherit instead of initial, so we can support IE. I also made the dialog wider on the medium breakpoint:
Two thoughts:
This PR is good to go, I'd say! Nice work. |
|
This looks great, and if @jasmussen says it looks good, I trust his judgement! Looks like there are a few tests failing, but other than that, anything blocking that needs to be done to get this ready? Is it realistic to get this in for the 5.7 deadline on friday? I'll get a11y to look at this this afternoon as well. |
|
Hey @hedgefield - the failing e2e tests are a Core regression tracked here: https://core.trac.wordpress.org/ticket/50025 |
|
Removing
This should be fixed now. I restarted the tests, let's see how it goes 🤞 |
I try! I really do 😄 I pushed a fix to make the back button and heading legible on mobile. The root cause is that the navigation component appears to be designed specifically for use in the dark side menu, to a point that changing this color and its inheritance is pretty difficult. So we really should separately make that component more color agnostic. |
Investigating this but they are not in a --edit |
|
I think the animated checkboxes are acceptable. I suspect it's because the CSS animation that happens starts/invokes when the item itself comes into being. |
Yes without the key React has know way of knowing that they're separate panels and so will update the existing checkbox in the DOM instead of destroying it / creating a new checkbox. |
|
Nice work all! |
Description
Resolves: #24965
This is a first stab at changing the
Preferencesmodal. Design was based from this comment: #24965 (comment).This PR requires feedback about design and technical implementation, regarding the usage of these components or creating new ones. Please don't focus too much on small code details for now, as this is in draft status.
Notes
We don't have a responsive component for handling the desired design(both large and smaller viewports), so it seems to me that the usage of different components cannot be avoided. For the larger viewports I used
TabPanelcomponent with a couple of new css rules and worked just fine.For the smaller viewports I used the
Navigationcomponent, which is also used in FSE navigation menu. Alternatively a new component must be created, unless there is one that I'm not aware of.In this PR
Block manageris NOT moved inPreferences, since it needs to be refactored with thepanelsdesign in the issue's mockup and also creates some extra issues with thestickystyling in combination with theModal. I believe it will make this PR unnecessarily bigger and can be done in a follow up.I haven't touched the tests yet (fixing and creating new ones and updating utils) and haven't taken care of the
active tabsync between the viewports.Feedback/help needed
Screenshots
Gifs are from starting plan with the
Block Managerwhich will be moved in a follow up.Larger Viewports
Smaller viewports
Checklist: