Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate post editor feature preferences to use new preferences package #39115

Merged
merged 4 commits into from
Mar 2, 2022
Merged

Migrate post editor feature preferences to use new preferences package #39115

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f73a32
Migrate post editor preferences to use new preferences package
talldan Feb 28, 2022
c79bdba
Fix css classes used in e2e tests
talldan Feb 28, 2022
a348603
Remove outdated note
talldan Mar 2, 2022
a0c0085
Re-alphabetize imports
talldan Mar 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion docs/reference-guides/data/data-core-edit-post.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,11 @@ _Returns_

### switchEditorMode

Undocumented declaration.
Triggers an action used to switch editor mode.

_Parameters_

- _mode_ `string`: The editor mode.

### toggleEditorPanelEnabled

Expand Down
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion packages/base-styles/_z-index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ $z-layers: (
// ...Except for popovers immediately beneath wp-admin menu on large breakpoints
".components-popover.block-editor-inserter__popover": 99999,
".components-popover.table-of-contents__popover": 99998,
".components-popover.edit-post-more-menu__content": 99998,
".components-popover.edit-site-more-menu__content": 99998,
".components-popover.interface-more-menu__content": 99998,
".components-popover.block-editor-rich-text__inline-format-toolbar": 99998,
Expand Down
5 changes: 4 additions & 1 deletion packages/data/src/plugins/persistence/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,10 @@ persistencePlugin.__unstableMigrate = ( pluginOptions ) => {
persistence,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@talldan talldan Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

'core/customize-widgets'
);
migrateFeaturePreferencesToInterfaceStore( persistence, 'core/edit-post' );
migrateFeaturePreferencesToPreferencesStore(
persistence,
'core/edit-post'
);
};

export default persistencePlugin;
2 changes: 1 addition & 1 deletion packages/e2e-test-utils/src/click-on-more-menu-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ")]',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we target using something that's user oriented e.g. ARIA label?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const SELECTORS = {
postEditorMenuContainer:
'//*[contains(concat(" ", @class, " "), " edit-post-more-menu__content ")]',
siteEditorMenuContainer:
'//*[contains(concat(" ", @class, " "), " edit-site-more-menu__content ")]',
};

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

siteEditorMenuContainer:
'//*[contains(concat(" ", @class, " "), " edit-site-more-menu__content ")]',
};
Expand Down
2 changes: 1 addition & 1 deletion packages/e2e-test-utils/src/toggle-more-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
const SELECTORS = {
postEditorMenuContent: '.interface-more-menu-dropdown__content',
siteEditorMenuContent: '.edit-site-more-menu__content',
postEditorMenu: '.edit-post-more-menu [aria-label="Options"]',
postEditorMenu: '.interface-more-menu-dropdown [aria-label="Options"]',
siteEditorMenu: '.edit-site-more-menu [aria-label="More tools & options"]',
};

Expand Down
6 changes: 3 additions & 3 deletions packages/e2e-tests/specs/editor/various/popovers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same again here. Could be a good opportunity to make these tests less dependent on implementation details such as class name.


expect( await isMoreMenuOpen() ).toBe( false );

// Toggle opened.
await page.click( '.edit-post-more-menu > button' );
await page.click( '.interface-more-menu-dropdown > button' );
expect( await isMoreMenuOpen() ).toBe( true );

// Toggle closed.
await page.click( '.edit-post-more-menu > button' );
await page.click( '.interface-more-menu-dropdown > button' );
expect( await isMoreMenuOpen() ).toBe( false );
} );
} );
Expand Down
11 changes: 6 additions & 5 deletions packages/e2e-tests/specs/editor/various/post-editor-template-mode.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,16 +230,17 @@ describe( 'Delete Post Template Confirmation Dialog', () => {
it( `should retain template if deletion is canceled when the viewport is ${ viewport }`, async () => {
await setBrowserViewport( viewport );

const isWelcomeGuideActive = await page.evaluate( () =>
wp.data
.select( 'core/edit-post' )
.isFeatureActive( 'welcomeGuide' )
const isWelcomeGuideActive = await page.evaluate(
() =>
!! wp.data
.select( 'core/preferences' )
.get( 'core/edit-post', 'welcomeGuide' )
);
if ( isWelcomeGuideActive === true ) {
await page.evaluate( () =>
wp.data
.dispatch( 'core/edit-post' )
.toggleFeature( 'welcomeGuide' )
.toggle( 'core/edit-post', 'welcomeGuide' )
);
await page.reload();
await page.waitForSelector( '.edit-post-layout' );
Expand Down
1 change: 1 addition & 0 deletions packages/edit-post/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"@wordpress/media-utils": "file:../media-utils",
"@wordpress/notices": "file:../notices",
"@wordpress/plugins": "file:../plugins",
"@wordpress/preferences": "file:../preferences",
"@wordpress/url": "file:../url",
"@wordpress/viewport": "file:../viewport",
"@wordpress/warning": "file:../warning",
Expand Down
8 changes: 1 addition & 7 deletions packages/edit-post/src/components/header/more-menu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
import { __ } from '@wordpress/i18n';
import { MenuGroup } from '@wordpress/components';
import {
ActionItem,
MoreMenuDropdown,
ActionItem,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this an accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it. I'll switch it back as I like alphabetical order.

PinnedItems,
} from '@wordpress/interface';
import { useViewportMatch } from '@wordpress/compose';
Expand All @@ -18,17 +18,11 @@ import PreferencesMenuItem from '../preferences-menu-item';
import ToolsMoreMenuGroup from '../tools-more-menu-group';
import WritingMenu from '../writing-menu';

const POPOVER_PROPS = {
className: 'edit-post-more-menu__content',
};

const MoreMenu = ( { showIconLabels } ) => {
const isLargeViewport = useViewportMatch( 'large' );

return (
<MoreMenuDropdown
className="edit-post-more-menu"
popoverProps={ POPOVER_PROPS }
toggleProps={ {
showTooltip: ! showIconLabels,
...( showIconLabels && { variant: 'tertiary' } ),
Expand Down
35 changes: 0 additions & 35 deletions packages/edit-post/src/components/header/more-menu/style.scss
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion packages/edit-post/src/components/header/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@
padding: 0 #{ $grid-unit-15 * 0.5 };
}

.edit-post-more-menu .components-button,
.interface-more-menu-dropdown .components-button,
.interface-pinned-items .components-button {
margin-right: 0;
}
Expand Down
14 changes: 7 additions & 7 deletions packages/edit-post/src/components/header/writing-menu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { MenuGroup } from '@wordpress/components';
import { __, _x } from '@wordpress/i18n';
import { useViewportMatch } from '@wordpress/compose';
import { displayShortcut } from '@wordpress/keycodes';
import { MoreMenuFeatureToggle } from '@wordpress/interface';
import { PreferenceToggleMenuItem } from '@wordpress/preferences';

function WritingMenu() {
const isLargeViewport = useViewportMatch( 'medium' );
Expand All @@ -15,27 +15,27 @@ function WritingMenu() {

return (
<MenuGroup label={ _x( 'View', 'noun' ) }>
<MoreMenuFeatureToggle
<PreferenceToggleMenuItem
scope="core/edit-post"
feature="fixedToolbar"
name="fixedToolbar"
label={ __( 'Top toolbar' ) }
info={ __(
'Access all block and document tools in a single place'
) }
messageActivated={ __( 'Top toolbar activated' ) }
messageDeactivated={ __( 'Top toolbar deactivated' ) }
/>
<MoreMenuFeatureToggle
<PreferenceToggleMenuItem
scope="core/edit-post"
feature="focusMode"
name="focusMode"
label={ __( 'Spotlight mode' ) }
info={ __( 'Focus on one block at a time' ) }
messageActivated={ __( 'Spotlight mode activated' ) }
messageDeactivated={ __( 'Spotlight mode deactivated' ) }
/>
<MoreMenuFeatureToggle
<PreferenceToggleMenuItem
scope="core/edit-post"
feature="fullscreenMode"
name="fullscreenMode"
label={ __( 'Fullscreen mode' ) }
info={ __( 'Work without distraction' ) }
messageActivated={ __( 'Fullscreen mode activated' ) }
Expand Down
4 changes: 2 additions & 2 deletions packages/edit-post/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { render, unmountComponentAtNode } from '@wordpress/element';
import { dispatch, select } from '@wordpress/data';
import { addFilter } from '@wordpress/hooks';
import { store as interfaceStore } from '@wordpress/interface';
import { store as preferencesStore } from '@wordpress/preferences';

/**
* Internal dependencies
Expand Down Expand Up @@ -106,7 +106,7 @@ export function initializeEditor(
initialEdits
);

dispatch( interfaceStore ).setFeatureDefaults( 'core/edit-post', {
dispatch( preferencesStore ).setDefaults( 'core/edit-post', {
fixedToolbar: false,
welcomeGuide: true,
fullscreenMode: true,
Expand Down
6 changes: 3 additions & 3 deletions packages/edit-post/src/plugins/welcome-guide-menu-item/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { MoreMenuFeatureToggle } from '@wordpress/interface';
import { PreferenceToggleMenuItem } from '@wordpress/preferences';
import { __ } from '@wordpress/i18n';

/**
Expand All @@ -17,9 +17,9 @@ export default function WelcomeGuideMenuItem() {
);

return (
<MoreMenuFeatureToggle
<PreferenceToggleMenuItem
scope="core/edit-post"
feature={ isTemplateMode ? 'welcomeGuideTemplate' : 'welcomeGuide' }
name={ isTemplateMode ? 'welcomeGuideTemplate' : 'welcomeGuide' }
label={ __( 'Welcome Guide' ) }
/>
);
Expand Down
12 changes: 8 additions & 4 deletions packages/edit-post/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import { castArray, reduce } from 'lodash';
*/
import { __ } from '@wordpress/i18n';
import apiFetch from '@wordpress/api-fetch';
import { speak } from '@wordpress/a11y';
import { store as interfaceStore } from '@wordpress/interface';
import { store as preferencesStore } from '@wordpress/preferences';
import { speak } from '@wordpress/a11y';
import { store as noticesStore } from '@wordpress/notices';
import { store as coreStore } from '@wordpress/core-data';
import { store as blockEditorStore } from '@wordpress/block-editor';
Expand Down Expand Up @@ -147,10 +148,13 @@ export function removeEditorPanel( panelName ) {
* @param {string} feature Feature name.
*/
export const toggleFeature = ( feature ) => ( { registry } ) =>
registry
.dispatch( interfaceStore )
.toggleFeature( 'core/edit-post', feature );
registry.dispatch( preferencesStore ).toggle( 'core/edit-post', feature );
Comment on lines 150 to +151
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Member

@noisysocks noisysocks Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Triggers an action used to switch editor mode.
*
* @param {string} mode The editor mode.
*/
export const switchEditorMode = ( mode ) => ( { dispatch, registry } ) => {
dispatch( {
type: 'SWITCH_MODE',
Expand Down
6 changes: 2 additions & 4 deletions packages/edit-post/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { get, includes, some, flatten, values } from 'lodash';
*/
import { createRegistrySelector } from '@wordpress/data';
import { store as interfaceStore } from '@wordpress/interface';
import { store as preferencesStore } from '@wordpress/preferences';
import { store as coreStore } from '@wordpress/core-data';
import { store as editorStore } from '@wordpress/editor';
/**
Expand Down Expand Up @@ -191,10 +192,7 @@ export function isModalActive( state, modalName ) {
*/
export const isFeatureActive = createRegistrySelector(
( select ) => ( state, feature ) => {
return select( interfaceStore ).isFeatureActive(
'core/edit-post',
feature
);
return !! select( preferencesStore ).get( 'core/edit-post', feature );
}
);

Expand Down
10 changes: 6 additions & 4 deletions packages/edit-post/src/store/test/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import { createRegistry } from '@wordpress/data';
import { store as interfaceStore } from '@wordpress/interface';
import { store as preferencesStore } from '@wordpress/preferences';
import { store as noticesStore } from '@wordpress/notices';
import { store as coreStore } from '@wordpress/core-data';
import { store as blockEditorStore } from '@wordpress/block-editor';
Expand All @@ -22,6 +23,7 @@ function createRegistryWithStores() {
blockEditorStore,
coreStore,
interfaceStore,
preferencesStore,
editorStore,
].forEach( registry.register );
return registry;
Expand Down Expand Up @@ -53,15 +55,15 @@ describe( 'actions', () => {
registry.dispatch( editPostStore ).toggleFeature( 'welcomeGuide' );
expect(
registry
.select( interfaceStore )
.isFeatureActive( editPostStore.name, 'welcomeGuide' )
.select( preferencesStore )
.get( editPostStore.name, 'welcomeGuide' )
).toBe( true );

registry.dispatch( editPostStore ).toggleFeature( 'welcomeGuide' );
expect(
registry
.select( interfaceStore )
.isFeatureActive( editPostStore.name, 'welcomeGuide' )
.select( preferencesStore )
.get( editPostStore.name, 'welcomeGuide' )
).toBe( false );
} );
describe( 'switchEditorMode', () => {
Expand Down
1 change: 0 additions & 1 deletion packages/edit-post/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
@import "./components/header/style.scss";
@import "./components/header/fullscreen-mode-close/style.scss";
@import "./components/header/header-toolbar/style.scss";
@import "./components/header/more-menu/style.scss";
@import "./components/header/template-title/style.scss";
@import "./components/keyboard-shortcut-help-modal/style.scss";
@import "./components/layout/style.scss";
Expand Down
4 changes: 2 additions & 2 deletions packages/nux/src/components/dot-tip/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,12 @@ $dot-scale: 3; // How much the pulse animation should scale up by in size
margin-right: 0;
}

&.components-popover.edit-post-more-menu__content:not([data-y-axis="middle"])[data-y-axis="right"] .components-popover__content {
&.components-popover.interface-more-menu-dropdown__content:not([data-y-axis="middle"])[data-y-axis="right"] .components-popover__content {
/*!rtl:ignore*/
margin-left: -12px;
}

&.components-popover.edit-post-more-menu__content:not([data-y-axis="middle"])[data-y-axis="left"] .components-popover__content {
&.components-popover.interface-more-menu-dropdown__content:not([data-y-axis="middle"])[data-y-axis="left"] .components-popover__content {
/*!rtl:ignore*/
margin-right: -12px;
}
Expand Down