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.

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

Navigation Editor: Allow menu renaming #29012

Merged
merged 23 commits into from
Mar 8, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import useMenuLocations from './use-menu-locations';
import useMenuLocations from '../../hooks/use-menu-locations';

export default function ManageLocations() {
const menus = useSelect( ( select ) => select( 'core' ).getMenus(), [] );
Expand Down
5 changes: 5 additions & 0 deletions packages/edit-navigation/src/components/header/save-button.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import { store as editNavigationStore } from '../../store';
import { useMenuEntity, MenuIdContext } from '../../hooks';
import { useContext } from '@wordpress/element';

export default function SaveButton( { navigationPost } ) {
const menuId = useContext( MenuIdContext );
const { saveMenuName } = useMenuEntity( menuId );
const { saveNavigationPost } = useDispatch( editNavigationStore );

return (
Expand All @@ -19,6 +23,7 @@ export default function SaveButton( { navigationPost } ) {
isPrimary
onClick={ () => {
saveNavigationPost( navigationPost );
saveMenuName();
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't anticipate this, but both these lines result in the menu being saved. saveNavigationPost saves the menu via the customizer API (because the REST API doesn't quite work yet for saving menu items).

I'd be worried about race conditions here.

This is some technical debt, so not really something caused by your PR.

I don't have a solution available right now, will have to have a think about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is the edge case when the race condition could really happen?

Copy link
Contributor

@talldan talldan Feb 23, 2021

Choose a reason for hiding this comment

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

I'm worried that one request might overwrite the other, since both API requests can modify the same database record.

I think the easiest solution right now would be to make sure the requests happen sequentially. This could be done by moving the saveEditedEntityRecord dispatch to be inside the saveNavigationPost action. Should be able to dispatch it with code like this, where the yield will wait for the request to complete before triggering the second request (the call to batchSave):

yield dispatch(
	'core',
	'saveEditedEntityRecord',
	'root',
	'menu',
	menuId
);

This makes sense actually, as it means the on-screen notices that are implemented in saveNavigationPost can be made so that the visible notices associated with saving are displayed at the right time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some further background. The way saving menus currently works is a temporary thing. Currently the batchSave function has been patched to use an endpoint that is currently used by the Customize > Menus screen. But that isn't part of the REST API, and not something we want to support long into the future for the navigation editor.

Ideally the whole system would use the WordPress REST APIs and the core-data package's entities. There are two things that need to happen:

} }
disabled={ ! navigationPost }
>
Expand Down
93 changes: 50 additions & 43 deletions packages/edit-navigation/src/components/layout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@ import { useRef } from '@wordpress/element';
* Internal dependencies
*/
import EmptyState from './empty-state';
import useNavigationEditor from './use-navigation-editor';
import useNavigationBlockEditor from './use-navigation-block-editor';
import useMenuNotifications from './use-menu-notifications';
import {
MenuIdContext,
useNavigationEditor,
useNavigationBlockEditor,
useMenuNotifications,
} from '../../hooks';
import ErrorBoundary from '../error-boundary';
import NavigationEditorShortcuts from './shortcuts';
import Header from '../header';
Expand Down Expand Up @@ -76,51 +79,55 @@ export default function Layout( { blockEditorSettings } ) {
'has-block-inspector': isBlockEditorReady,
} ) }
>
<Header
isPending={ ! hasLoadedMenus }
menus={ menus }
selectedMenuId={ selectedMenuId }
onSelectMenu={ selectMenu }
navigationPost={ navigationPost }
/>
<MenuIdContext.Provider value={ selectedMenuId }>
<Header
isPending={ ! hasLoadedMenus }
menus={ menus }
selectedMenuId={ selectedMenuId }
onSelectMenu={ selectMenu }
navigationPost={ navigationPost }
/>

{ ! hasFinishedInitialLoad && <Spinner /> }
{ ! hasFinishedInitialLoad && <Spinner /> }

{ hasFinishedInitialLoad && ! hasMenus && (
<EmptyState />
) }
{ hasFinishedInitialLoad && ! hasMenus && (
<EmptyState />
) }

{ isBlockEditorReady && (
<BlockEditorProvider
value={ blocks }
onInput={ onInput }
onChange={ onChange }
settings={ {
...blockEditorSettings,
templateLock: 'all',
} }
useSubRegistry={ false }
>
<BlockEditorKeyboardShortcuts />
<NavigationEditorShortcuts
saveBlocks={ savePost }
/>
<div
className="edit-navigation-layout__canvas"
ref={ canvasRef }
{ isBlockEditorReady && (
<BlockEditorProvider
value={ blocks }
onInput={ onInput }
onChange={ onChange }
settings={ {
...blockEditorSettings,
templateLock: 'all',
} }
useSubRegistry={ false }
>
<Editor
isPending={ ! hasLoadedMenus }
blocks={ blocks }
<BlockEditorKeyboardShortcuts />
<NavigationEditorShortcuts
saveBlocks={ savePost }
/>
<div
className="edit-navigation-layout__canvas"
ref={ canvasRef }
>
<Editor
isPending={ ! hasLoadedMenus }
blocks={ blocks }
/>
</div>
<InspectorAdditions
menuId={ selectedMenuId }
onDeleteMenu={ deleteMenu }
/>
<BlockInspector
bubblesVirtually={ false }
/>
</div>
<InspectorAdditions
menuId={ selectedMenuId }
onDeleteMenu={ deleteMenu }
/>
<BlockInspector bubblesVirtually={ false } />
</BlockEditorProvider>
) }
</BlockEditorProvider>
) }
</MenuIdContext.Provider>
</div>

<Popover.Slot />
Expand Down
35 changes: 35 additions & 0 deletions packages/edit-navigation/src/components/name-editor/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useEffect, useState } from '@wordpress/element';
/**
* Internal dependencies
*/
import { BlockControls } from '@wordpress/block-editor';
import { ToolbarGroup, ToolbarItem } from '@wordpress/components';
import { useMenuEntity, useNavigationEditorMenu } from '../../hooks';

export function NameEditor() {
const { menuName, menuId } = useNavigationEditorMenu();
const { editMenuName } = useMenuEntity( menuId );
talldan marked this conversation as resolved.
Show resolved Hide resolved
const [ tmpMenuName, setTmpMenuName ] = useState( menuName );
grzim marked this conversation as resolved.
Show resolved Hide resolved
useEffect( () => setTmpMenuName( menuName ), [ menuName ] );
return (
<>
<BlockControls>
draganescu marked this conversation as resolved.
Show resolved Hide resolved
<ToolbarGroup>
<ToolbarItem
as="input"
value={ tmpMenuName }
onChange={ ( { target: { value } } ) => {
setTmpMenuName( value );
editMenuName( value );
} }
aria-label={ __( 'Edit menu name' ) }
/>
</ToolbarGroup>
</BlockControls>
</>
);
}
33 changes: 33 additions & 0 deletions packages/edit-navigation/src/filters/add-menu-name-editor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* WordPress dependencies
*/
/**
* Internal dependencies
*/
import { NameEditor } from '../components/name-editor';
import { addFilter } from '@wordpress/hooks';
import { createHigherOrderComponent } from '@wordpress/compose';
import { useNavigationEditor } from '../hooks';

const addMenuNameEditor = createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
if ( props.name !== 'core/navigation' ) {
return <BlockEdit { ...props } />;
}
const { menuName } = useNavigationEditor();
return (
<>
<BlockEdit { ...props } menuName={ menuName } />
<NameEditor { ...props } />
</>
);
},
'withMenuName'
);

export default () =>
addFilter(
'editor.BlockEdit',
'core/edit-navigation/with-menu-name',
addMenuNameEditor
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* WordPress dependencies
*/
import { addFilter } from '@wordpress/hooks';
/**
* External dependencies
*/
import { set } from 'lodash';

function disableInsertingNonNavigationBlocks( settings, name ) {
if ( ! [ 'core/navigation', 'core/navigation-link' ].includes( name ) ) {
set( settings, [ 'supports', 'inserter' ], false );
}
return settings;
}

export default () =>
addFilter(
'blocks.registerBlockType',
'core/edit-navigation/disable-inserting-non-navigation-blocks',
disableInsertingNonNavigationBlocks
);
18 changes: 18 additions & 0 deletions packages/edit-navigation/src/filters/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Internal dependencies
*/
import addMenuNameEditor from './add-menu-name-editor';
import disableInsertingNonNavigationBlocks from './disable-inserting-non-navigation-blocks';
import removeEditUnsupportedFeatures from './remove-edit-unsupported-features';
import removeSettingsUnsupportedFeatures from './remove-settings-unsupported-features';

export const addFilters = (
shouldAddDisableInsertingNonNavigationBlocksFilter
) => {
addMenuNameEditor();
if ( shouldAddDisableInsertingNonNavigationBlocksFilter ) {
disableInsertingNonNavigationBlocks();
}
removeEditUnsupportedFeatures();
removeSettingsUnsupportedFeatures();
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* WordPress dependencies
*/
import { addFilter } from '@wordpress/hooks';
import { createHigherOrderComponent } from '@wordpress/compose';

const removeNavigationBlockEditUnsupportedFeatures = createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
if ( props.name !== 'core/navigation' ) {
return <BlockEdit { ...props } />;
}

return (
<BlockEdit
{ ...props }
hasSubmenuIndicatorSetting={ false }
hasItemJustificationControls={ false }
hasListViewModal={ false }
/>
);
},
'removeNavigationBlockEditUnsupportedFeatures'
);

export default () =>
addFilter(
'editor.BlockEdit',
'core/edit-navigation/remove-navigation-block-edit-unsupported-features',
removeNavigationBlockEditUnsupportedFeatures
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* WordPress dependencies
*/
import { addFilter } from '@wordpress/hooks';

function removeNavigationBlockSettingsUnsupportedFeatures( settings, name ) {
if ( name !== 'core/navigation' ) {
return settings;
}

return {
...settings,
supports: {
customClassName: false,
html: false,
inserter: true,
},
// Remove any block variations.
variations: undefined,
};
}

export default () =>
addFilter(
'blocks.registerBlockType',
'core/edit-navigation/remove-navigation-block-settings-unsupported-features',
removeNavigationBlockSettingsUnsupportedFeatures
);
15 changes: 15 additions & 0 deletions packages/edit-navigation/src/hooks/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { createContext } from '@wordpress/element';

export const untitledMenu = __( '(untitled menu)' );
export const MenuIdContext = createContext();

export { default as useMenuEntity } from './use-menu-entity';
export { default as useNavigationEditor } from './use-navigation-editor';
export { default as useNavigationBlockEditor } from './use-navigation-block-editor';
export { default as useMenuNotifications } from './use-menu-notifications';
export { default as useNavigationEditorMenu } from './use-navigation-editor-menu';
export { default as useMenuLocations } from './use-menu-locations';
39 changes: 39 additions & 0 deletions packages/edit-navigation/src/hooks/use-menu-entity.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* WordPress dependencies
*/
import { useSelect, useDispatch } from '@wordpress/data';
/**
* External dependencies
*/
import { isUndefined, negate } from 'lodash';
/**
* Internal dependencies
*/
import { MENU_KIND, MENU_POST_TYPE } from '../utils/constants';

import { untitledMenu } from './index';

export default function useMenuEntity( menuId ) {
const { editEntityRecord, saveEditedEntityRecord } = useDispatch( 'core' );

const menuEntityData = [ MENU_KIND, MENU_POST_TYPE, menuId ];
const editedMenu = useSelect(
( select ) =>
select( 'core' ).getEditedEntityRecord( ...menuEntityData ),
[ menuId ]
);

const editedMenuName = menuId && editedMenu.name;

const saveMenuName = () =>
negate( isUndefined )( editedMenuName ) &&
saveEditedEntityRecord( ...menuEntityData );

const editMenuName = ( name = untitledMenu ) =>
editEntityRecord( ...menuEntityData, { name } );

return {
saveMenuName,
editMenuName,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,21 @@
import { useSelect, useDispatch } from '@wordpress/data';
import { useEffect } from '@wordpress/element';
import { store as noticesStore } from '@wordpress/notices';
/**
* Internal dependencies
*/
import { MENU_POST_TYPE, MENU_KIND } from '../utils/constants';

export default function useMenuNotifications( menuId ) {
const { lastSaveError, lastDeleteError } = useSelect(
( select ) => ( {
lastSaveError: select( 'core' ).getLastEntitySaveError(
'root',
'menu'
MENU_KIND,
MENU_POST_TYPE
),
lastDeleteError: select( 'core' ).getLastEntityDeleteError(
'root',
'menu',
MENU_KIND,
MENU_POST_TYPE,
menuId
),
} ),
Expand Down
Loading