-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
3ebd509
feature to change navigation menu name
b5ee729
name editor component added
d3ce125
add missing styles|
2295c5a
remove doubled hooks
19ea70c
remove ;
2f0531f
navigation post variable updated
2b55320
use-menu-locations hook moved to hooks directory
416f946
console error fixed
e6b0992
name edition moved to the sidebar. In toolbar manu name changed to bu…
e724897
move NameEditor to InspectorControls in InspectorAdditions in order t…
bec25b5
use TextControl component in name-editor
1c8a9a2
changes according to CR
0a90dda
add missing file
bbfbff1
changes according to CR: removed aria-label from name-display/index.j…
4073eda
Dispatch save requests sequentially instead of in parallel
talldan fcdab0a
explicitly set () => setIsMenuNameEditFocused( true )
e6415be
add translation to the file
2dfb3ad
resolved conflicts with trunk
d3cb3df
resolved conflicts with trunk 2
cd98168
Merge branch 'trunk' into add/navigation-menu-name-editor
grzim 00561eb
sprintf added
98dc6a0
Merge branch 'add/navigation-menu-name-editor' of github.com:WordPres…
2f53f2f
Update unit test to take into account the call to saveEditedEntityRecord
talldan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
24 changes: 24 additions & 0 deletions
24
packages/edit-navigation/src/components/name-display/index.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { ToolbarGroup, ToolbarButton } from '@wordpress/components'; | ||
import { BlockControls } from '@wordpress/block-editor'; | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { useNavigationEditorMenu, IsMenuEditorFocused } from '../../hooks'; | ||
import { useContext } from '@wordpress/element'; | ||
|
||
export default function NameDisplay() { | ||
const { menuName } = useNavigationEditorMenu(); | ||
const [ , setIsMenuNameEditFocused ] = useContext( IsMenuEditorFocused ); | ||
return ( | ||
<BlockControls> | ||
<ToolbarGroup> | ||
<ToolbarButton onClick={ setIsMenuNameEditFocused }> | ||
grzim marked this conversation as resolved.
Show resolved
Hide resolved
grzim marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ menuName } | ||
</ToolbarButton> | ||
</ToolbarGroup> | ||
</BlockControls> | ||
); | ||
} |
66 changes: 66 additions & 0 deletions
66
packages/edit-navigation/src/components/name-editor/index.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
import { useEffect, useRef, useState, useContext } from '@wordpress/element'; | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { TextControl } from '@wordpress/components'; | ||
import { | ||
IsMenuEditorFocused, | ||
useMenuEntity, | ||
useNavigationEditorMenu, | ||
} from '../../hooks'; | ||
import { useInstanceId } from '@wordpress/compose'; | ||
|
||
export function NameEditor() { | ||
const [ isMenuNameEditFocused, setIsMenuNameEditFocused ] = useContext( | ||
IsMenuEditorFocused | ||
); | ||
|
||
const { menuName, menuId } = useNavigationEditorMenu(); | ||
const { editMenuName } = useMenuEntity( menuId ); | ||
talldan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const inputRef = useRef(); | ||
const [ tmpMenuName, setTmpMenuName ] = useState( menuName ); | ||
grzim marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const instanceId = useInstanceId( NameEditor ); | ||
const id = `components-edit-navigation-name-editor__input-${ instanceId }`; | ||
useEffect( () => setTmpMenuName( menuName ), [ menuName ] ); | ||
useEffect( () => { | ||
if ( isMenuNameEditFocused ) inputRef.current.focus(); | ||
}, [ isMenuNameEditFocused ] ); | ||
return ( | ||
<> | ||
<TextControl | ||
ref={ inputRef } | ||
help={ __( | ||
'A short, descriptive name used to refer to this menu elsewhere.' | ||
) } | ||
label={ __( 'Name' ) } | ||
id={ id } | ||
talldan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
onBlur={ () => setIsMenuNameEditFocused( false ) } | ||
className="components-name-editor__text-control" | ||
grzim marked this conversation as resolved.
Show resolved
Hide resolved
|
||
value={ tmpMenuName } | ||
onChange={ ( value ) => { | ||
setTmpMenuName( value ); | ||
editMenuName( value ); | ||
} } | ||
aria-label={ __( 'Edit menu name' ) } | ||
grzim marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/> | ||
{ /*<div>*/ } | ||
{ /* <input*/ } | ||
{ /* ref={ inputRef }*/ } | ||
{ /* id={ id }*/ } | ||
{ /* onBlur={ () => setIsMenuNameEditFocused( false ) }*/ } | ||
{ /* className="components-text-control__input"*/ } | ||
{ /* value={ tmpMenuName }*/ } | ||
{ /* onChange={ ( { target: { value } } ) => {*/ } | ||
{ /* setTmpMenuName( value );*/ } | ||
{ /* editMenuName( value );*/ } | ||
{ /* } }*/ } | ||
{ /* aria-label={ __( 'Edit menu name' ) }*/ } | ||
{ /* />*/ } | ||
{ /*</div>*/ } | ||
grzim marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</> | ||
); | ||
} |
11 changes: 11 additions & 0 deletions
11
packages/edit-navigation/src/components/name-editor/style.scss
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
.edit-navigation-name-editor__edit-name-description { | ||
font-size: $helptext-font-size; | ||
font-style: normal; | ||
color: $gray-700; | ||
} | ||
|
||
grzim marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.components-name-editor__text-control { | ||
grzim marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.components-base-control { | ||
border: none; | ||
} | ||
} |
31 changes: 31 additions & 0 deletions
31
packages/edit-navigation/src/filters/add-menu-name-editor.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { addFilter } from '@wordpress/hooks'; | ||
import { createHigherOrderComponent } from '@wordpress/compose'; | ||
import NameDisplay from '../components/name-display'; | ||
|
||
const addMenuNameEditor = createHigherOrderComponent( | ||
( BlockEdit ) => ( props ) => { | ||
if ( props.name !== 'core/navigation' ) { | ||
return <BlockEdit { ...props } />; | ||
} | ||
return ( | ||
<> | ||
<BlockEdit { ...props } /> | ||
<NameDisplay /> | ||
</> | ||
); | ||
}, | ||
'withMenuName' | ||
); | ||
|
||
export default () => | ||
addFilter( | ||
'editor.BlockEdit', | ||
'core/edit-navigation/with-menu-name', | ||
addMenuNameEditor | ||
); |
22 changes: 22 additions & 0 deletions
22
packages/edit-navigation/src/filters/disable-inserting-non-navigation-blocks.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
}; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 thesaveNavigationPost
action. Should be able to dispatch it with code like this, where theyield
will wait for the request to complete before triggering the second request (the call tobatchSave
):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.There was a problem hiding this comment.
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: