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 Block: fix link UI popover only opens if there's one navigation block in the page #47829

Closed
MaggieCabrera opened this issue Feb 7, 2023 · 4 comments · Fixed by #48219
Assignees
Labels
[Block] Navigation Link Affects the Navigation Link Block [Block] Navigation Affects the Navigation Block [Type] Bug An existing feature does not function as intended

Comments

@MaggieCabrera
Copy link
Contributor

Description

While fixing #47828 we uncovered a new bug in the case where more than one navigation block is present on the page.

Step-by-step reproduction instructions

Insert a navigation block with custom links.
Click the inserter to add a new custom link, it should trigger the popover to edit the link.
Insert a new navigation block just below.
Insert a new custom link to it. The popover should open, but it doesn't.

Screenshots, screen recording, code snippet

Screen Capture on 2023-02-07 at 12-21-54

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@getdave
Copy link
Contributor

getdave commented Feb 13, 2023

Problem

We have x2 Navigation blocks which are both referencing the same underlying wp_navigation entity. Therefore changing items in Nav block 1 will automatically see those changes reflected in Nav block 2 via syncing with the entity.

When a link block is inserted into menu 1 the lastBlockInserted state reflects that link block which exists in menu 1 (and nav block 1).

But then immediately after that Nav block 2 syncs with the entity which triggers REPLACE_INNER_BLOCKS which causes the state for lastBlockInserted to update to reflect the synced blocks and not the block that was manually inserted by the user.

What we want is for the user changes only to be reported as the "last block updated" whereas what's happening is the synced blocks are reported. This means that the following check will never pass...

...and thus and Link UI is never shown in the list view.

Possible Solutions

Add "actor" metadata

In #47874 we considered adding a actor to the metadata associated with the REPLACE_ and INSERT_ actions and storing that in the lastInsertedBlock state. We then thought we'd attach an actor of user to any actions triggered by a user (i.e. using the inserter) and an actor of auto to any actions triggered automatically (e.g. useBlockSync).

The idea was that we'd grab the meta from the state and discard any results that were triggered by auto actors. However this won't work because the state is wiped out for each new action, meaning if you discard auto then you can't continue looking "back" in the state for blocks inserted by user because these have been removed by subsequent actions.

Other ideas

  • Maybe have the state store all the blocks that have been inserted for the current editor session. Then traversing history to look for the last block inserted matching a given actor would be possible.

@scruffian
Copy link
Contributor

I think this bug is a manifestation of the fragile nature of the solution we have here. I don't love the current UX for adding links to a navigation block - I think that the idea being explored in #46981 is a superior UX and we should explore this direction.

This bug only happens in quite a specific scenario (you need to have the same nav block twice in the same page) and even then it doesn't stop you from being able to edit the block - it just requires a few extra clicks - you could even argue that it's a better experience than the current one!

All that is to say I think we should pause these efforts and put them into #46981.

@scruffian
Copy link
Contributor

I don't think this is anything to do with lastInsertedBlockClientId. When I remove this line, the problem goes away:

setAttributes( { isTopLevelLink } );

@scruffian
Copy link
Contributor

I think we can solve this a different way: #48219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Link Affects the Navigation Link Block [Block] Navigation Affects the Navigation Block [Type] Bug An existing feature does not function as intended
Projects
None yet
4 participants