Add reusable preferences modal to interface package. #39153
Conversation
|
Size Change: +4.65 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
tellthemachines
requested review from
noisysocks,
talldan,
youknowriad and
jorgefilipecosta
|
It'd be great to add some docs for this, even if it's just a small example. |
|
|
||
| const PREFERENCES_MENU = 'preferences-menu'; | ||
|
|
||
| export default function PreferencesModal( { closeModal, sections } ) { |
There was a problem hiding this comment.
A drawback of having the modal component directly accept the sections array for its content is that it makes it impossible to add any freeform content before or after the content (e.g. if there was ever a use case to show an inline notice below the modal header or some kind of footer at the bottom).
An option would be to have another component that acts as the sections:
<PreferencesModal>
// other stuff can be added above or below the sections if needed in the future.
<PreferencesSections sections={ sections } />
</PreferencesModal>There was a problem hiding this comment.
True; that would also solve the problem of what happens if there's only one section, as it won't make much sense rendering tabs in that case. PreferencesSections could just be responsible for the tabbed interface; maybe best call it PreferencesTabs to make that more explicit. (Also because we already have PreferencesModalSection that goes inside each tab 😅 )
There was a problem hiding this comment.
Or PreferencesModalTabs to keep to the naming pattern 😄
There was a problem hiding this comment.
Updated this PR and the testing branch with new component.
| function BaseOption( { help, label, isChecked, onChange, children } ) { | ||
| return ( | ||
| <div className="interface-preferences-modal__option"> | ||
| <ToggleControl |
There was a problem hiding this comment.
Once #39158 is merged, a next step will be to add a PreferencesToggleControl to the preferences package (similar to the way PreferencesToggleMenuItem works).
I'm not sure what the best way to integrate it with this generic toggle control will be 🤔
The preferences package could use this component, but then that becomes only useful in the preferences modal (and I don't think preferences should depend on interface).
There was a problem hiding this comment.
The only reason to have this wrapper around ToggleControl is to allow children to be attached to it. If we implement a PreferencesToggleControl with that capacity, we don't need this component at all.
There was a problem hiding this comment.
Maybe we should start with this one being __unstable and then see how the other toggle component evolves.
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
There was a problem hiding this comment.
Just a small note, I think the interface package is fine here but I also wonder whether these are just high-level UI components (so @wordpress/components). cc @ciampo
👍 either way :)
This component looks quite high-level and specific to the preferences UI (and it looks like it already uses many components from |
| $vertical-tabs-width: 160px; | ||
|
|
||
| .interface-preferences__tabs { | ||
| .components-tab-panel__tabs { |
There was a problem hiding this comment.
Ideally we should try to avoid using internal components classnames (as they are not regarded as "public APIs") and to override styles this way, as those styles can easily break in case the component is updated in the future.
I understand this may also be caused by a limitation in TabPanel's design, or by its misalignment with the recent UI direction — cc'ing @jasmussen here who may have more insights here
There was a problem hiding this comment.
Thanks for your feedback @ciampo !
These styles were copied verbatim from the original edit-post modal, so they've been around for a while now. For components-tab-panel__tabs and components-modal__content I could potentially target them by aria-role instead - would this be preferable?
The problem is components-base-control__help is a p tag with no distinctive features, so there's no other way to target it. (By the way: that's an accessibility issue, because the contents of that tag are ignored by screen readers. It should be linked to the control through an aria-describedby or similar. I'm happy to create an issue for that if there isn't one already 🙂 )
In more general terms, are there any thoughts around expanding the components API to allow applying styles to child elements of a component? It's quite frequent across the codebase that we resort to the internal classnames for this end. It would be good to find a solution, e.g.:
- make some of those classnames an official part of the API;
- provide a way to pass classnames for child elements (this can become messy though)
- provide a CSS custom property API whereby consumers can set certain values for the child elements (colors, spacing etc.) of a component.
Let me know what you think!
There was a problem hiding this comment.
These styles were copied verbatim from the original edit-post modal, so they've been around for a while now.
Thank you for the explanation, I previously had missed this detail (my fault, I skipped the PR's description and went straight to the ping from Riad). I'd say, let's try to see if there's any low-hanging fruit (like this comment) and leave more complicated refactors for another time!
For
components-tab-panel__tabsandcomponents-modal__contentI could potentially target them by aria-role instead - would this be preferable?
That would be slightly better, but we can probably leave it as-is in this PR. I would be personally questioning why we need to override so many styles for the Modal and TabPanel components — the better solution in my opinion would be to avoid these styles overrides as much as possible. In a follow-up PR we could look at what limitations we're facing when using these components, and see how we can overcome them (by either improving these components or using a different set of components).
The problem is components-base-control__help is a p tag with no distinctive features, so there's no other way to target it. (By the way: that's an accessibility issue, because the contents of that tag are ignored by screen readers. It should be linked to the control through an aria-describedby or similar. I'm happy to create an issue for that if there isn't one already 🙂 )
Understood — feel free to create an issue for that, and tag @mirka and me.
In more general terms, are there any thoughts around expanding the components API to allow applying styles to child elements of a component? It's quite frequent across the codebase that we resort to the internal classnames for this end. It would be good to find a solution, e.g.:
- make some of those classnames an official part of the API;
- provide a way to pass classnames for child elements (this can become messy though)
- provide a CSS custom property API whereby consumers can set certain values for the child elements (colors, spacing etc.) of a component.
On this topic, we think that hardcoded classnames and internal DOM structure should not be part of the public APIs of components, as they would severely limit the amount of internal (private) changes that can be made to the components; most internal changes would likely cause breakages for the consumers of such components.
I also agree that exposing more props for children classnames is not the correct solution, as it would scale very poorly.
Regarding CSS custom properties, it's something that we're going to take a look at soon when we'll start actively working on introducing a lightweight theming API layer.
Another approach that we could have is to try and be more "modular" with our components — e.g. expose a BaseControl and a BaseControlHelp components that the consumer of the library would need to use together (rather than exporting one monolithic BaseControl component).
cc @mirka
| @media (max-width: #{ ($break-medium - 1) }) { | ||
| .components-modal__content { | ||
| padding: 0; | ||
|
|
||
| &::before { | ||
| content: none; | ||
| } | ||
| } | ||
| // Keep the navigator component from overflowing the modal content area | ||
| // to ensure that sticky position elements stick where intended. | ||
| .components-navigator-provider { | ||
| height: 100%; | ||
| } | ||
| } | ||
|
|
||
| .components-base-control__help { | ||
| margin: -$grid-unit-10 0 $grid-unit-10 58px; | ||
| font-size: $helptext-font-size; | ||
| font-style: normal; | ||
| color: $gray-700; | ||
| } |
There was a problem hiding this comment.
Same here re. using hardcoded, internal components classnames (see previous message)
There was a problem hiding this comment.
Great work @tellthemachines! This is looking good to me:
✅ Confirmed by testing #39176 that the preferences modal in the post editor looks and functions exactly the same as on trunk, across its four breakpoints.
✅ Included documentation reads well.
✅ Confirmed the logic and styling copied over is largely the same as what's in edit-post, with a couple of minor changes 👍 — I agree with the discussions that there's room for improvement in how some of the styling is structured, but it'd be better to pursue those changes outside of this refactor.
LGTM! 🎉
priethor
added
the
[Type] New API
Description
Adds a reusable preferences modal to the interface package, so it can be used across editors. It's based on the post editor preferences modal; I essentially copied all the boilerplate across and changed the classnames.
One thing I did change was the markup for the modal sections: it's now using a
fieldsetandlegendso that the section title and description can be identified by screen readers.Testing Instructions
#39176 adds this modal to the post editor, so to test this it's best to check out that branch and follow testing instructions on that PR.
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).