Editor: Unify right click override preference #57468
Conversation
youknowriad
added
[Type] Enhancement
|
Size Change: +1.54 kB (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice consolidation, it feels really good for this feature to be consistent between the two editors.
✅ Toggling in the post editor works and the change is reflected in the site editor
✅ Toggling in the site editor works and the change is reflected in the post editor
✅ Other settings are unaffected for now (block breadcrumbs, always open list view), though could be good candidates for follow-ups
I also like the way this treats the right click feature as canonically belonging to 'core/edit-post', seems like a good way to handle it to me.
LGTM! ✨
|
Is the use of |
|
I wasn't aware there was a plan to unify the preferences, but I think it's a good idea. Have any other preferences been migrated?
I'd also prefer it to be I think the issue is that a migration would have to be undertaken to 'move' where the preferences are in local storage and user meta. They're always a pain to write. |
|
Great points! A central / common namespace does sound like a good idea.
Looks like this is the first PR, but the others are being proposed here: #52632 (comment) ? |
Thanks, I'll make sure to comment there. I just remembered that the 'core' scope has already been used for the hints that show up in the editor ( Has the right click preference shipped in (m)any plugin releases? Given it's new and opt-out, it's probably not the end of the world if it's changed to 'core' without a migration. I doubt many will have actively decided to switch it off. |
It's slated for Gutenberg 17.4 so it hasn't made it to a published release yet.
Same. Also I think the impact would be pretty minimal if folks needed to switch it off again, so I personally don't think we'd need to worry about a migration for this feature. |
|
Personally, I think it's fine to use core/edit-post scope for these for now to avoid migrations. in the end it's just a string, maybe we should document this somehow to explain the reasoning though. And yes, although this is the first explicit PR that I do to migrate preferences, there's already a precedent in using I don't mind a migration but I think we should keep it as a separate task to migrate all the preferences at once (if we ever feel the need at some point). |
True, though I worry that it may be confusing if there's a transition period where But moving on, the idea would be that post editor preferences take precedence over any other? A migration might still be good at the end for cleaning up old data in user meta. I think at the moment updated preferences are merged into the preferences object, and the whole payload is sent to the use meta endpoint, so the unused data would stick around.
That preference already existed in the editor package, but I think it was only used in the post editor so that's why it was |
| @@ -6,16 +6,21 @@ import { ___unstablePreferencesModalBaseOption as BaseOption } from '@wordpress/ | |||
| import { store as preferencesStore } from '@wordpress/preferences'; | |||
|
|
|||
| export default function EnableFeature( props ) { | |||
| const { featureName, onToggle = () => {}, ...remainingProps } = props; | |||
| const { | |||
| namespace = 'core/edit-site', | |||
There was a problem hiding this comment.
Nit: The prop could be called scope to match what preferences calls it.
But also I wonder if there could be one component in the editor package for this that has 'core/edit-post' hardcoded. Any migrated preferences could use the editor package component, then eventually the edit-site and edit-post ones could be deprecated/removed.
There was a problem hiding this comment.
Yes, I think we need to unify the UI down the road which would mean updating this component a bit. I think for now it's fine to make this change in the component. I'll update to use scope.
|
I also think that not using the |
|
Ok, in the last commit I update the scope to "core" and added a migration, let me know if I did everything properly. |
...s/preferences-persistence/src/migrations/preferences-package-data/convert-editor-settings.js
Outdated
Show resolved
Hide resolved
| const settingsToMoveToCore = [ 'allowRightClickOverrides' ]; | ||
|
|
||
| settingsToMoveToCore.forEach( ( setting ) => { | ||
| if ( data?.[ 'core/edit-post' ]?.[ setting ] !== undefined ) { |
There was a problem hiding this comment.
Should we do the same for core/edit-site store? Maybe first check if it's already set in core scope..
There was a problem hiding this comment.
absence of "core/edit-post" value doesn't really mean we don't have a preference, it just means that it was not explicitly set. So I think we should just keep it simple as is.
| delete newData[ 'core/edit-post' ][ setting ]; | ||
| } | ||
| } ); | ||
|
|
||
| if ( Object.keys( newData?.[ 'core/edit-post' ] ?? {} )?.length === 0 ) { | ||
| delete newData[ 'core/edit-post' ]; | ||
| } |
There was a problem hiding this comment.
For example, does it make sense to remove the same setting in the core/edit-site store as shown below?
delete newData[ 'core/edit-site' ][ setting ];
if ( Object.keys( newData?.[ 'core/edit-site' ] ?? {} )?.length === 0 ) {
delete newData[ 'core/edit-site' ];
}There was a problem hiding this comment.
Good catch, forgot to do that.
There was a problem hiding this comment.
LGTM. Thank you for addressing the comments (and for generally undertaking this work). I tested out the migration manually, and it works well (for anyone testing, you have to change a preference to save the migrated data):
The comment about deleting site editor data as well also seems relevant, but given there are already PRs for migrating other preferences, it could also be tackled on one of those.
| @@ -17,11 +17,19 @@ export default function convertEditorSettings( data ) { | |||
| }; | |||
| delete newData[ 'core/edit-post' ][ setting ]; | |||
| } | |||
|
|
|||
| if ( data?.[ 'core/edit-post' ]?.[ setting ] !== undefined ) { | |||
There was a problem hiding this comment.
Should it be 'core/edit-site' here?
There was a problem hiding this comment.
Though potentially the preference for the site editor can be deleted at the same time as the data for the post editor (after line 18) given it's being disregarded and no longer used for anything.
There was a problem hiding this comment.
oops, right, I'll fix it in one of the other PRs
There was a problem hiding this comment.
It can't be on line 18 though because it might exist while the post editor one won't.
There was a problem hiding this comment.
Fix included here 429a995
Related #52632
What?
This PR continues the work on the great unification between post and site editors. In this PR we're unifying the "right click override" preference. If the user allows right click overrides in the post editor editor, the setting should be used in the site editor as well. IMO all these editor preferences should be common to both editors.
Testing instructions