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

Navigation: Save theme location on button click #30340

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Navigation: Save theme location on button click #30340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion packages/edit-navigation/src/components/name-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { useEffect, useRef, useContext } from '@wordpress/element';
import { TextControl } from '@wordpress/components';
import {
IsMenuNameControlFocusedContext,
untitledMenu,
useMenuEntity,
useSelectedMenuData,
} from '../../hooks';
Expand All @@ -19,7 +20,14 @@ export function NameEditor() {
);

const { menuId } = useSelectedMenuData();
const { editMenuName, editedMenuName } = useMenuEntity( menuId );
const { editedMenu, editMenuEntityRecord, menuEntityData } = useMenuEntity(
menuId
);
const editedMenuName = menuId && editedMenu.name;

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

const inputRef = useRef();
useEffect( () => {
if ( isMenuNameEditFocused ) inputRef.current.focus();
Expand Down
12 changes: 3 additions & 9 deletions packages/edit-navigation/src/hooks/use-menu-entity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import { useSelect, useDispatch } from '@wordpress/data';
*/
import { MENU_KIND, MENU_POST_TYPE } from '../constants';

import { untitledMenu } from './index';

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

Expand All @@ -20,13 +18,9 @@ export default function useMenuEntity( menuId ) {
[ menuId ]
);

const editedMenuName = menuId && editedMenu.name;

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

return {
editedMenuName,
editMenuName,
editedMenu,
menuEntityData,
editMenuEntityRecord: editEntityRecord,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can expose this as a function that takes a menu id and menu data as an alternative to returning menuEntityData. The MENU_KIND and MENU_POST_TYPE aren't expected to change.

I also discovered recently that there's a useEntityProp hook, that works similarly to useState, so that wrapping that in some way might also be an option.

Copy link
Contributor

@talldan talldan Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I guess to elaborate more, I'd consider menuEntityData as configuration that can be kept internal to useMenuEntity.

Instead of returning it the hook could return a partially applied version of editEntityRecord so that implementers don't have to worry about passing the MENU_KIND and MENU_POST_TYPE arguments.

Just to also mention that there are other ways of achieving this too.

  • We might consider moving some of this to the edit-navigation store—an action in the navigation store can dispatch actions on another store. Selectors can also select from other stores. We could expose an editMenuName action and a getEditedMenuName selector for convenience.

  • Or there's the useEntityProp hook that I mentioned (https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/src/entity-provider.js#L74-L117), that provides an easier way to edit individual properties. We could also consider partially applied versions (e.g useMenuEntityProp, or maybe even useMenuEntityName/useMenuEntityLocation) of this hook for convenience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a separate issue for this #30442

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of useMenuEntityProp. Happy to create PR and update existing code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent @Mamaduka if you find the time to squeeze that it would be lovely 🙏

Copy link
Member

Choose a reason for hiding this comment

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

Done #31132 ✅

};
}
25 changes: 13 additions & 12 deletions packages/edit-navigation/src/hooks/use-menu-locations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,26 @@
*/
import { useState, useEffect, useMemo, useCallback } from '@wordpress/element';
import apiFetch from '@wordpress/api-fetch';
import { useDispatch } from '@wordpress/data';

/**
* External dependencies
*/
import { merge } from 'lodash';
/**
* Internal dependencies
*/
import { useMenuEntity, useSelectedMenuData } from './index';

const locationsForMenuId = ( menuLocationsByName, id ) =>
Object.values( menuLocationsByName )
.filter( ( { menu } ) => menu === id )
.map( ( { name } ) => name );

const withLocations = ( menuLocationsByName ) => ( id ) => ( {
id,
locations: locationsForMenuId( menuLocationsByName, id ),
} );

export default function useMenuLocations() {
const [ menuLocationsByName, setMenuLocationsByName ] = useState( null );

const { menuId } = useSelectedMenuData();
const { editMenuEntityRecord, menuEntityData } = useMenuEntity( menuId );
useEffect( () => {
let isMounted = true;

Expand All @@ -41,8 +41,6 @@ export default function useMenuLocations() {
return () => ( isMounted = false );
}, [] );

const { saveMenu } = useDispatch( 'core' );

const assignMenuToLocation = useCallback(
async ( locationName, newMenuId ) => {
const oldMenuId = menuLocationsByName[ locationName ].menu;
Expand All @@ -53,10 +51,13 @@ export default function useMenuLocations() {

setMenuLocationsByName( newMenuLocationsByName );

[ oldMenuId, newMenuId ]
.filter( Boolean )
.map( withLocations( newMenuLocationsByName ) )
.forEach( saveMenu );
const activeMenuId = oldMenuId || newMenuId;
editMenuEntityRecord( ...menuEntityData, {
locations: locationsForMenuId(
newMenuLocationsByName,
activeMenuId
),
} );
},
[ menuLocationsByName ]
);
Expand Down