Add new @wordpress/preferences package
#38873
Conversation
talldan
added
the
[Type] New API
talldan
requested review from
tellthemachines,
a team,
fullofcaffeine,
youknowriad and
gziolo
and removed request for
a team
| * @param {string} scope The feature scope (e.g. core/edit-post). | ||
| * @param {string} featureName The feature name. | ||
| */ | ||
| export function toggleFeature( scope, featureName ) { |
There was a problem hiding this comment.
In the future I imagine we'll have not only boolean preferences in this store but also other types. Probably an 'enum' style preference would be next.
I considered whether there should just be one function for setting any kind of preference, but I think a toggle style function is quite a nice convenience compared to a more freeform setPreferenceValue, so I've kept this API similar to the way it is now.
Renaming it from 'feature' is something we might consider though.
| * | ||
| * @return {Object} Action object. | ||
| */ | ||
| export function setFeatureDefaults( scope, defaults ) { |
There was a problem hiding this comment.
In contrast to my previous comment, I think this function could set the defaults for both boolean and enum style settings. So perhaps this is something we might rename to setDefaults.
Thoughts?
Maybe I should also make everything experimental to start with.
talldan
force-pushed
the
create/new-preferences-package
branch
from
d1ee33a to
8c0be41
Compare
|
Size Change: 0 B Total Size: 1.15 MB ℹ️ View Unchanged
|
|
What's the rationale behind moving also React components into this package? I can see how a customizable button component with a preference feature toggle wired would be useful to use in different places. The one proposed in this PR is tied to the More Menu on editor screens, so it's going to be harder to use with 3rd party projects or WordPress plugins. Maybe we should keep those components in |
| } | ||
| ``` | ||
|
|
||
| ## API Reference |
There was a problem hiding this comment.
Should we take this opportunity to simplify further. What I have in mind could be something like:
- drop the "feature" name (as it's a bit weird) and just call them preferences.
- Basically the main selectors could be just
getandset
There was a problem hiding this comment.
Agreed, the 'feature' thing doesn't really fit for a generic preferences package. I'll work on the naming. 👍
I think it might good to have a toggle too, most of the preferences are booleans, so it seems like a handy function to have.
On this subject, I've been wondering if we want any built-in type safety for these preferences. Or whether it should just be a freeform key/value store. What do you think?
There was a problem hiding this comment.
On this subject, I've been wondering if we want any built-in type safety for these preferences. Or whether it should just be a freeform key/value store. What do you think?
If we go the type safety road, it almost feels like we should "register" preferences. I'm not sure it's worth the added complexity. I understand that if we don't do it from the start it might harder to do later. Any use-cases that benefits from the type safety?
There was a problem hiding this comment.
Any use-cases that benefits from the type safety?
There definitely aren't many in the editor, so I think you're right that it's not worth the complexity. The only one really is the Code Editor/Visual Editor option.
There is also the option of building more complex behavior around the existing actions and selectors. For example, if an editor needed a preference with some kind of validation, it could add that validation in its own action that calls through to dispatch( 'core/preferences' ).set.
| } ); | ||
|
|
||
| // Once we build a more generic persistence plugin that works across types of stores | ||
| // we'd be able to replace this with a register call. |
There was a problem hiding this comment.
It feels like a good opportunity to basically deprecated the persistence plugin and have its logic handled directly by the current package.
There was a problem hiding this comment.
(not necessary for the first release of the package)
There was a problem hiding this comment.
Yep, that would be great. Just checked and it looks like there's one other usage in production - enableItems in the interface package.
There was a problem hiding this comment.
Great initiative, we need such package :)
- If I'm not wrong, this is not used yet, I'm curious to see how the migration of existing preferences would work.
- I'm curious to see how we can adapt the package in the future to persist using an API (should this be something you can provide as an adapter or built-in in the package)
@gziolo Mostly for convenience, but they could also stay in the interface package. I hadn't really thought of that.
@youknowriad There's another PR (https://github.com/WordPress/gutenberg/pull/38435/files#diff-3201447415d9f0cf361527ea43e669de47f38c3d8df5a848e86a3cdaf21fe057L234) that demonstrates the migration that can also be used for manual testing (and I'm also using it for the automated tests while developing). This PR is a subset of that one that only contains the preference store changes. |
packages/preferences/README.md
Outdated
|
|
||
| ### Components | ||
|
|
||
| The `MoreMenuDropdown` and `MoreMenuPreferenceToggle` components help to implement an editor menu for changing preferences and feature values. |
There was a problem hiding this comment.
While I think MoreMenuPreferenceToggle makes sense in this package (probably better renamed as just PreferenceToggle), I have some trouble understanding the need of MoreMenuDropdown in the context of the preferences package.
There was a problem hiding this comment.
I think it makes sense to have MoreMenuDropdown here because it ensures the dropdown classname, associated styles in packages/preferences/src/components/more-menu-dropdown/style.scss and button icon are all defined in one place, and don't have to be added to each package where preferences are used.
There was a problem hiding this comment.
This sounds like sharing code just to share code and avoid duplication. In general, I feel like this might not always be the right argument for code sharing as it makes us create wrong abstractions. I'm fine with a sharable DropdownMenu but this one particularly sounds more suited for the "interface" package instead. It has nothing to do with "preferences".
There was a problem hiding this comment.
👍 Interface sounds like a good option. After all there's a bunch of options in this menu that are not related to preferences.
Some of the plugin extensibility that's loosely connected with the dropdown lives in interface, so that works.
There was a problem hiding this comment.
That's all done now. I've renamed MoreMenuPreferenceToggle to PreferenceToggleMenuItem. It's a bit longer than the name suggested, but in future we may add another toggle component for the preferences modal.
There was a problem hiding this comment.
This looks promising! Just a couple minor comments below.
I notice in the demo PR the site editor isn't updated to use this package. Is that just because it isn't using the current common component in interface, or is there any other obstacle to migrating it?
Also, considering this question, would it make sense to bring the post editor PreferencesModal into this package too? We could at least leverage MoreMenuPreferenceToggle for some of its options.
| @@ -0,0 +1,172 @@ | |||
| # Preferences | |||
|
|
|||
| Utilities for storing WordPress preferences. | |||
There was a problem hiding this comment.
Should this be "editor" instead of "WordPress" preferences?
There was a problem hiding this comment.
Why should we limit this to "editor"? That's for sure the main use-case for us but does it prevent us to make this package ready for any client-side rendered WP-Admin page?
There was a problem hiding this comment.
Good point, but should we restrict it to WordPress? This package isn't inherently tied to WP, unless I'm missing something. It could potentially be used by non-WP block editors too.
There was a problem hiding this comment.
That's true. It may depend on how the persistence functionality is implemented in the future, as at some point we'll want to persist settings to the database via the REST API rather than localstorage.
If that can be done in a way where it's configurable or separate to the package (which I think it probably should be), it would be fine to drop the 'WordPress' from this, but for now I don't think it's harmful.
packages/preferences/README.md
Outdated
|
|
||
| ### Components | ||
|
|
||
| The `MoreMenuDropdown` and `MoreMenuPreferenceToggle` components help to implement an editor menu for changing preferences and feature values. |
There was a problem hiding this comment.
I think it makes sense to have MoreMenuDropdown here because it ensures the dropdown classname, associated styles in packages/preferences/src/components/more-menu-dropdown/style.scss and button icon are all defined in one place, and don't have to be added to each package where preferences are used.
| const { toggle } = useDispatch( preferencesStore ); | ||
| const speakMessage = () => { | ||
| if ( isActive ) { | ||
| speak( messageDeactivated || __( 'Preference deactivated' ) ); |
There was a problem hiding this comment.
Should the default message use the label string to give a bit more context, instead of just saying "Preference"? Otherwise we risk multiple preferences having the exact same message.
There was a problem hiding this comment.
Sounds good. I've made that change in dfbdf41
talldan
force-pushed
the
create/new-preferences-package
branch
from
8b15daa to
64ed509
Compare
talldan
force-pushed
the
create/new-preferences-package
branch
from
d26afcc to
dfbdf41
Compare
@tellthemachines The site editor isn't using the code that was previously added to
It could, perhaps either here or interface. Unfortunately that won't be quick. I need to start again on #34195, that code is pretty outdated. It's also a huge change, so will definitely be a separate PR. |
|
I think I've addressed all the feedback. Let me know if there's any more. |
| label={ __( 'My feature' ) } | ||
| info={ __( 'A really awesome feature' ) } | ||
| messageActivated={ __( 'My feature activated' ) } | ||
| messageDeactivated={ __( 'My feature deactivated' ) } |
There was a problem hiding this comment.
I think this is not new to this PR but in other components that needed configurable labels, we had a single object prop like messages in FormTokenField. Feels like something we'd need to normalize at some point.
There was a problem hiding this comment.
I can take a look at that in a follow-up. 👍
|
That is really nice! Great work @talldan 🚀 |
| { | ||
| "name": "@wordpress/preferences", | ||
| "version": "1.0.0-prerelease", | ||
| "private": true, |
There was a problem hiding this comment.
Other public packages depend on this package so it shouldn't be private. See #39390.
Description
First part of #31965
This PR contains part of the code from #38435, I've separated it out for easier review and will keep the two in sync.
It adds a new
@wordpress/preferencespackage with a data store and a few components.Here's some links to the documentation for everything this PR introduces:
PreferenceToggleMenuItemcomponent docsThis is a good time to review the API and do any renaming. At the moment, everything works similarly to the way it does in
trunk, but there is scope now to change things.Currently the only preferences managed in the new package are what we call 'feature' preferences, which are just simple boolean flags. Surprisingly this covers almost all the preferences in our editors, with the exception of a couple of things:
Testing Instructions
For testing use the
add/new-preferences-packagebranch from #38435. Everything should behave the same astrunkin the post editor, widgets editor and customize widgets editor which I've refactored in that PR to use the new preferences store.Types of changes
New package (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).