Migrate post editor feature preferences to use new preferences package #39115
Conversation
talldan
added
[Type] Enhancement
talldan
requested review from
ajitbohra,
ntwb,
nerrad,
mkaz and
ryanwelcher
as code owners
| export const toggleFeature = ( feature ) => ( { registry } ) => | ||
| registry | ||
| .dispatch( interfaceStore ) | ||
| .toggleFeature( 'core/edit-post', feature ); | ||
| registry.dispatch( preferencesStore ).toggle( 'core/edit-post', feature ); |
There was a problem hiding this comment.
There is the option of deprecating the existing actions and selectors in edit-post.
There's quite a lot of usage across the codebase (and possibly in plugins too), so I haven't done that. I'd be happy to address that in a follow-up if reviewers feel a deprecation is best.
There was a problem hiding this comment.
Yes I think we should deprecate it. Otherwise future devs will mistakenly use this one. Since it is a method that made it into Core, we can't remove the method, but that's okay.
There was a problem hiding this comment.
What form could that deprecation take?
I'm thinking we could add some sort of message in the actions and selectors but we might never be able to remove them altogether without risk of breaking things for plugins. It would be good to update usage in this codebase anyway, to make sure we're setting the right example (definitely a follow-up job though!)
There was a problem hiding this comment.
Yes because it shipped in Core we have to follow the Core deprecation policy which is to call deprecated() (or any of the deprecated_* methods in PHP) but never actually remove the method. It will spit out a a warning but continue working.
|
Size Change: +795 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
talldan
changed the title
|
Something to note is that there are still some persisted preferences left in the post editor after this change: gutenberg/packages/edit-post/src/store/reducer.js Lines 47 to 124 in 0f73a32 Here's the list: Panel preferencesThis stores and persists whether a panel is open or closed. This is probably more of an Block PreferencesThere are a coupe of different types of block preferences— Should we consider using I feel like it be concerned only with more general WordPress settings, but I'd be interested to hear other thoughts. Local autosave interval and editor mode (visual or code editor)These both seem like ok things to store in the new preferences packages. They'll require some custom migration code for local storage data, so I think I'll do this in a separate PR. |
talldan
force-pushed
the
migrate/edit-post-to-use-new-preferences
branch
from
6cc033f
to
c79bdba
Compare
|
@talldan In this comment #39115 (comment) you seem to suggest that some preferences are not suited for the "preferences" package. Can you help understand why is that? and how to know if it's suitable or not. I guess for me, all preferences should be handled by the package. |
|
@youknowriad I'm wondering where we draw the line on some things. I think there are some parts of the post editor's For example the I think this kind of use case (UI state) is quite a common one. IIRC there's been some similar feedback about the design tools (#38219 (comment)). At the same time, the interface store also has a similar feature for storing UI state, which it uses to keep track of pinned items. But maybe the answer is to also make everything in interface use preferences too. At that point we're using |
I guess that was my initial thinking: a package used to persist and interact with preferences rather than a package that defines the "WordPress preferences" that can be accessed/edited. In a sense, the difference between "@wordpress/data" and "@wordpress/core-data". I see the preferences package as the "data" package (the generic layer) but not the "core-data" (the usage of the data package to access WP data) |
I think it's ok for it to evolve in that way, it's pretty much a blank canvas right now. I'll work on some separate PRs for those preferences mentioned and then we can see how that looks and whether we want to change the API in any way 👍 |
Yeah this makes sense to me. I don't think we'd ever need |
| @@ -359,7 +359,10 @@ persistencePlugin.__unstableMigrate = ( pluginOptions ) => { | |||
| persistence, | |||
There was a problem hiding this comment.
A few lines above this we have a comment about WordPress 6.0. What's up with that? I imagine this stuff needs to stay around forever, no?
There was a problem hiding this comment.
Hmm, I don't know what the policy is, that note is definitely outdated though, I'll remove it for now. There were some previous very old migrations in the past that had a note to remove them, so I did that. Was that a bad idea? 😄
Maybe it's worth being a bit more careful now that there are a few preferences that affect accessibility.
Previously the functionality tied to this was fairly minor (fullscreen mode, top toolbar).
There was a problem hiding this comment.
It depends what the migrations did 😀
If e.g. WordPress 5.0 stored something in localStorage in an old format then WordPress 6.0 needs to be able to migrate that.
If it was only ever in the Gutenberg plugin then we can safely delete the migration after a few versions.
There was a problem hiding this comment.
(Mind you, "needs" is a strong word, it's not very impactful when migration doesn't work as localStorage tends to be cleared by the user periodically anyway.)
| @@ -14,7 +14,7 @@ import { toggleMoreMenu } from './toggle-more-menu'; | |||
|
|
|||
| const SELECTORS = { | |||
| postEditorMenuContainer: | |||
| '//*[contains(concat(" ", @class, " "), " edit-post-more-menu__content ")]', | |||
| '//*[contains(concat(" ", @class, " "), " interface-more-menu-dropdown__content ")]', | |||
There was a problem hiding this comment.
Can we target using something that's user oriented e.g. ARIA label?
There was a problem hiding this comment.
Could be tricky as the block toolbar has an identical label for its settings menu.
I would like to get rid of this logic that uses a different class name for the site editor:
gutenberg/packages/e2e-test-utils/src/click-on-more-menu-item.js
Lines 15 to 20 in 11ae04c
I'll be able to remove that once I've done the migration for the site editor, so at that point I'll loop back and improve this.
There was a problem hiding this comment.
In my experience user-facing labels change more often than classnames 😅
It's always a compromise with e2e selectors; it's not a huge deal if we have to change them once in a while. Classnames also tend to be more unique than aria-labels.
| @@ -11,16 +11,16 @@ describe( 'popovers', () => { | |||
| describe( 'dropdown', () => { | |||
| it( 'toggles via click', async () => { | |||
| const isMoreMenuOpen = async () => | |||
| !! ( await page.$( '.edit-post-more-menu__content' ) ); | |||
| !! ( await page.$( '.interface-more-menu-dropdown__content' ) ); | |||
There was a problem hiding this comment.
Same again here. Could be a good opportunity to make these tests less dependent on implementation details such as class name.
| export const toggleFeature = ( feature ) => ( { registry } ) => | ||
| registry | ||
| .dispatch( interfaceStore ) | ||
| .toggleFeature( 'core/edit-post', feature ); | ||
| registry.dispatch( preferencesStore ).toggle( 'core/edit-post', feature ); |
There was a problem hiding this comment.
Yes I think we should deprecate it. Otherwise future devs will mistakenly use this one. Since it is a method that made it into Core, we can't remove the method, but that's okay.
There was a problem hiding this comment.
Code looks good and tests OK! Is the plan to merge this one first or wait until #39132 is ready?
| MoreMenuDropdown, | ||
| ActionItem, |
There was a problem hiding this comment.
Was this an accident?
There was a problem hiding this comment.
Seems like it. I'll switch it back as I like alphabetical order.
| export const toggleFeature = ( feature ) => ( { registry } ) => | ||
| registry | ||
| .dispatch( interfaceStore ) | ||
| .toggleFeature( 'core/edit-post', feature ); | ||
| registry.dispatch( preferencesStore ).toggle( 'core/edit-post', feature ); |
There was a problem hiding this comment.
What form could that deprecation take?
I'm thinking we could add some sort of message in the actions and selectors but we might never be able to remove them altogether without risk of breaking things for plugins. It would be good to update usage in this codebase anyway, to make sure we're setting the right example (definitely a follow-up job though!)
I'll merge this first now it's approved. Thanks for the reviews ❤️ |
Description
Addresses some of #31965, follows #39112
Migrates the post editor to use the new preferences package (introduced in #38873). Previously it was using interface, but the plan is now to deprecate the preferences APIs in interface in favor of this new package.
The main goal here is that nothing is different from a user perspective.
Testing Instructions
trunkopen the post editor and dismiss the welcome guideChecklist:
*.native.jsfiles for terms that need renaming or removal).