Refactor editor 'feature' preferences to interface package #33774
Conversation
talldan
added
[Type] New API
|
Size Change: +1.3 kB (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
| <MenuGroup label={ _x( 'View', 'noun' ) }> | ||
| <FeatureToggle | ||
| <MoreMenuFeatureToggle | ||
| scope="core/edit-widgets" |
There was a problem hiding this comment.
Having to define core/edit-widgets everywhere gets a bit repetitive. An option could be a ScopeProvider as part of the interface package.
There was a problem hiding this comment.
A local component might help, too:
const MoreMenuWidgetsFeatureToggle = ( props ) => <MoreMenuFeatureToggle { ...props } scope="core/edit-widgets />;I think that it's how this issue is mitigated in the edit post package. It had some backward compatiblity considerations, so we can explore other ideas, too.
There was a problem hiding this comment.
I like the idea of using a Provider. We can embed the provider inside <InterfaceSkeleton> seamlessly unless there're needs to use the components outside the tree.
An alternative could be to use hook-like API to re-implement this.
// edit-widgets/src/components/preference.js
const Preference = createPreference( 'core/edit-widgets', defaultPreferences );
export const PreferenceProvider = Preference.Provider; // The provider component
export const getPreference = Preference.get; // Getter function to get the preference data
export const setPreference = Preference.set; // Setter function to set the preference data
export const usePreference = Preference.usePreference; // A hook to react to changes and be used in componentsSo that all descendent components can use the hook to automatically get the preference data without having to specify scope.
A downside of this API is that it's not an idiomatic @wordpress/data-style selector we're used to, so it might be harder for third-party developers to hook into.
I'm in favor of the current approach, just want to throw out some alternative approaches to see if it can bring up some ideas from others.
There was a problem hiding this comment.
I like the idea of using a Provider. We can embed the provider inside seamlessly unless there're needs to use the components outside the tree.
Yeah, this is exactly what I was thinking. It would still be possible for scope to be overridden if a prop is provided to a context using component, but I wouldn't see that being a common use case.
There are other components within the interface package that use scope, so I'd have to double-check how they work, but I can work on a separate proof of concept.
I also think it's worth seeing how this will work for editors that have more settings, edit/widgets isn't really a great example of this being a challenge, it may be that @gziolo's suggestion is easier and makes more sense in the post editor.
One point on that is that context won't help for the selectors or actions, the solutions there would be a custom react hook, or alternatively registry selectors / generator actions. There's existing evidence of that here in the edit-post package:
gutenberg/packages/edit-post/src/store/actions.js
Lines 31 to 51 in 5a610d4
There was a problem hiding this comment.
If some of the options are shared across editors, we'll be using more than one scope here. Defining the scope manually might the best way to go until we've got this working everywhere.
There was a problem hiding this comment.
I think it'd still be ok with any of the approaches. It could be a separate component that doesn't use the context (GlobalFeatureToggle), or a prop that overrides the scope from context.
We had a migration logic present in the past. It operated on the local storage level and it would copy settings between namespaces. The only challenge was that it had to take into account that the same settings could be consumed with the plugin enabled and disabled. If I recall correctly, that prevented clean-up of copied settings. |
gziolo
added
the
[Feature] Extensibility
|
Looks like there's this code for handling migrations: gutenberg/packages/data/src/plugins/persistence/index.js Lines 225 to 230 in 5e24b5c The comment mentions an intention to remove this, but seems like it would be straightforward to repurpose it for this PR. |
|
This is almost ready for review. There's a bug at the moment where the first time a value is toggled it's from |
talldan
requested review from
adamziel,
ajitbohra,
draganescu,
kevin940726,
nerrad,
noisysocks,
ntwb and
tellthemachines
as code owners
|
This is all working now, so marking as ready for review 😄 |
talldan
force-pushed
the
refactor/editor-preferences-to-interface-package
branch
from
cba0892 to
904d9b4
Compare
talldan
changed the title
talldan
changed the title
tellthemachines
left a comment
tellthemachines
left a comment
There was a problem hiding this comment.
Looks good! I left a bunch of questions, mostly about cross-editor feature sharing as this will help #24370 along.
| persistence.set( interfaceStoreName, { | ||
| preferences: { | ||
| features: { | ||
| [ sourceStoreName ]: sourceFeatures, |
There was a problem hiding this comment.
Will we at a later stage be consolidating the features that are to be shared across editors (not necessarily all of them) under a "shared" key or something?
There was a problem hiding this comment.
This aspect wasn't something I'd considered when working on this task, but it's absolutely possible after this change now that the values are stored in one place.
I think the challenge will be how we reconcile that a user might already have conflicting values for preferences across editors. If I have Top Toolbar active in the post editor, but not active in customize widgets, which one is right?
Other than that, I don't think the implementation of a shared preference would be difficult now, I can think of a few different ways to do it, it all depends on how we'd want it to work. Would it be as simple as one value to rule them all, or might we allow a combination of a global and local preferences?
Some thoughts:
- It might be that some preferences are singleton, they use a single 'scope' via a shared key
- It might be that there are still different scopes, but the user is able to change them all to one value in a single action (e.g. a global setting in the WordPress settings menu)
- It might be that a global setting acts more like a default value, but the user can still override it in an individual editor
There was a problem hiding this comment.
These are all great points.
Regarding global vs local, I was thinking for some options - mostly the a11y/usability related ones, such as text labels on buttons and keeping the cursor inside the block - it make sense to just set them globally, as it's unlikely someone would want to use them in only one editor. For the others, maybe a global option with local override, or local-only makes more sense.
I initially thought of site settings as the best place to put global options, but might be good to get some design input on that too.
packages/interface/src/components/more-menu-feature-toggle/index.js
Outdated
Show resolved
Hide resolved
| <MenuGroup label={ _x( 'View', 'noun' ) }> | ||
| <FeatureToggle | ||
| <MoreMenuFeatureToggle | ||
| scope="core/edit-widgets" |
There was a problem hiding this comment.
If some of the options are shared across editors, we'll be using more than one scope here. Defining the scope manually might the best way to go until we've got this working everywhere.
packages/edit-widgets/src/components/widget-areas-block-editor-provider/index.js
Show resolved
Hide resolved
talldan
force-pushed
the
refactor/editor-preferences-to-interface-package
branch
from
904d9b4 to
cc6edb6
Compare
|
Feedback should all be addressed, and this is ready for another round of reviews again. |
talldan
force-pushed
the
refactor/editor-preferences-to-interface-package
branch
from
6c511c7 to
9f58d3a
Compare
|
Thanks for the reviews, I believe everything is addressed, so merging now 🚢 |
|
Thank you @talldan for working on this refactoring. This is a great step towards providing a unified UI for different editor screens 💯 |
Description
Partly addresses #31965
A WIP PR for generalising the concept of editor preferences so that implementing them across editors is easier.
So far all the boolean preferences in the standalone widgets editor have been migrated.
I've moved this to the interface package, but it could also be a separate package. It does seem to fit into the interface package's remit.
Tasks:
Follow ups:
How has this been tested?
General testing
Testing that preferences in local storage are migrated to
interface.WP_DATA_USER)trunkand rebuildScreenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).