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

Only request fallback navigation menu if necessary #52775

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from
+12 −8
Draft

Only request fallback navigation menu if necessary #52775

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
20 changes: 12 additions & 8 deletions packages/block-library/src/navigation/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ function Navigation( {
const [ overlayMenuPreview, setOverlayMenuPreview ] = useState( false );

const {
navigationMenus,
hasResolvedNavigationMenus,
isNavigationMenuResolved,
isNavigationMenuMissing,
Expand Down Expand Up @@ -226,16 +227,18 @@ function Navigation( {

const { getNavigationFallbackId } = unlock( useSelect( coreStore ) );

const navigationFallbackId = ! ( ref || hasUnsavedBlocks )
? getNavigationFallbackId()
: null;

useEffect( () => {
// If:
// - we haven't resolved the menus request and there isn't one missing, OR
// - there is an existing menu, OR
// - there are existing (uncontrolled) inner blocks
// ...then don't request a fallback menu.
if ( ref || hasUnsavedBlocks || ! navigationFallbackId ) {
if (
! navMenuResolvedButMissing ||
navigationMenus.length > 0 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

"Fallback" is an interesting concept. It doesn't mean "create a fallback". I mean "get the most appropriate menu to use as a fallback".

This will run

  • if there are no menus at all
  • if there are menus but this particular nav block does not have a ref

Prior to this we just had lots of "use the first menu" scattered all over the place so the fallback selector helps to unify this.

That said, if the selector doesn't immediately return data from state, then it will trigger a resolver which is async and thus we do need to conditionalise 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.

Hmmm... So we need to make a request for the fallback if there are no menus, or if we don't know what menu they want. So, it was decided to be better UX to show some menu over an error/no menu?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's it. So we want to avoid a "no menus" situation.

So we

  • get the most recently created Navigation Menu post (if available). If not we fall back to...
  • convert a most recently created Classic Menu to a Navigation Menu and use that. If no classic menu we fall back to...
  • create a Navigation Menu and use that.

That logic is all server side in a dedicated class that is called from the fallback endpoint.

ref ||
hasUnsavedBlocks
) {
return;
}

Expand All @@ -244,14 +247,15 @@ function Navigation( {
* The fallback should not request a save (entity dirty state)
* nor to be undoable, hence why it is marked as non persistent
*/

__unstableMarkNextChangeAsNotPersistent();
setRef( navigationFallbackId );
setRef( getNavigationFallbackId() );
Copy link
Contributor

Choose a reason for hiding this comment

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

The first time this runs the ref may end up being set to null because the selector has an async resolver.

}, [
ref,
setRef,
hasUnsavedBlocks,
navigationFallbackId,
navigationMenus,
navMenuResolvedButMissing,
getNavigationFallbackId,
__unstableMarkNextChangeAsNotPersistent,
] );

Expand Down