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: Update deps for the useEffect that creates navigation menus #47912

Merged
merged 3 commits into from
Feb 10, 2023
Merged

Navigation: Update deps for the useEffect that creates navigation menus #47912

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd2b54f
Update deps for the useEffect that creates navigation menus
scruffian Feb 9, 2023
586ecf4
still check for draft menus
scruffian Feb 9, 2023
0b15056
remvove unneeded code
scruffian Feb 10, 2023
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
64 changes: 29 additions & 35 deletions packages/block-library/src/navigation/edit/unsaved-inner-blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,37 +91,34 @@ export default function UnsavedInnerBlocks( {
}
);

const { isSaving, draftNavigationMenus, hasResolvedDraftNavigationMenus } =
useSelect(
( select ) => {
if ( isDisabled ) {
return EMPTY_OBJECT;
}

const {
getEntityRecords,
hasFinishedResolution,
isSavingEntityRecord,
} = select( coreStore );

return {
isSaving: isSavingEntityRecord(
'postType',
'wp_navigation'
),
draftNavigationMenus: getEntityRecords(
...DRAFT_MENU_PARAMS
),
hasResolvedDraftNavigationMenus: hasFinishedResolution(
'getEntityRecords',
DRAFT_MENU_PARAMS
),
};
},
[ isDisabled ]
);

const { hasResolvedNavigationMenus, navigationMenus } = useNavigationMenu();
const { isSaving, hasResolvedDraftNavigationMenus } = useSelect(
( select ) => {
if ( isDisabled ) {
return EMPTY_OBJECT;
}

const {
getEntityRecords,
hasFinishedResolution,
isSavingEntityRecord,
} = select( coreStore );

return {
isSaving: isSavingEntityRecord( 'postType', 'wp_navigation' ),
draftNavigationMenus: getEntityRecords(
// This is needed so that hasResolvedDraftNavigationMenus gives the correct status.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to keep this line, otherwise hasResolvedDraftNavigationMenus doesn't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just not check if navigation menus are resolved? We only need them because

Also ensure other navigation menus have loaded so an
		// accurate name can be created.

Is there another way? Like wait for this when creating the default title instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably but i'm not sure how to do that!

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 don't think so actually - we also need to check if draft navigations are created because navigation we create is a draft. If I remove the check for hasResolvedDraftNavigationMenus then two get created.

Copy link
Contributor

Choose a reason for hiding this comment

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

When we call createNavigationMenu we do it with an empty title and there is a hook useGenerateDefaultNavigationTitle where we alreadt check that we loaded all drafts already so we can assign the correct number.

			getEntityRecords( ...DRAFT_MENU_PARAMS ),
			getEntityRecords( ...PUBLISHED_MENU_PARAMS ),
		] );

That means this guard here is kinda pointless.

Copy link
Contributor

@getdave getdave Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why don't you pass in the isCreatingNavigationMenu status of the createNavigationMenu from the edit component? That way we have a way of telling whether the conversion is in progress. That might help us block the 2nd menu because it gives a true status of the progress of the event we care about - namely: whether we are already creating a navigation.

Relying on various resolved states is surely prone to error.

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't stop us calling createNavigationMenu twice though does it?

I didn't mean that. I mean that hasResolvedDraftNavigationMenus and hasResolvedNavigationMenus are only needed here as deps because we check for them in the effect - but createNavigationMenu which is the only result of the effect ALSO checks them - it seems they're not needed. And if they're not needed we don't need draftNavigationMenus either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove them then the problem comes back, so I think they are needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why don't you pass in the isCreatingNavigationMenu status of the createNavigationMenu from the edit component?

I don't think this will work. After the new navigation is created, the status is reset to idle, but the block is still dirty as the new (draft) navigation hasn't yet loaded, so the effect gets triggered again, and we end up in a loop creating endless new menus.

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 didn't mean that. I mean that hasResolvedDraftNavigationMenus and hasResolvedNavigationMenus are only needed here as deps because we check for them in the effect - but createNavigationMenu which is the only result of the effect ALSO checks them - it seems they're not needed. And if they're not needed we don't need draftNavigationMenus either.

I don't think this will work, because although createNavigationMenu does check the status of those dependencies, it happens after we call createNavigationMenu, so the useEffect that is checking whether the blocks are in a dirty state will continue to be called.

...DRAFT_MENU_PARAMS
),
hasResolvedDraftNavigationMenus: hasFinishedResolution(
'getEntityRecords',
DRAFT_MENU_PARAMS
),
};
},
[ isDisabled ]
);

const { hasResolvedNavigationMenus } = useNavigationMenu();

// Automatically save the uncontrolled blocks.
useEffect( () => {
Expand Down Expand Up @@ -154,11 +151,8 @@ export default function UnsavedInnerBlocks( {
isSaving,
hasResolvedDraftNavigationMenus,
hasResolvedNavigationMenus,
draftNavigationMenus,
navigationMenus,
innerBlocksAreDirty,
hasSelection,
createNavigationMenu,
blocks,
] );

const Wrapper = isSaving ? Disabled : 'div';
Expand Down