Fix inconsistent Link UI in Nav block list view editor #50774
Merged
Conversation
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 comment was marked as resolved.
This comment was marked as resolved.
|
Size Change: +21.1 kB (+2%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
getdave
force-pushed
the
fix/nav-list-view-unwanted-link-ui
branch
from
ee79a82
to
836504b
Compare
|
Flaky tests detected in 8c6122c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5027578549
|
This comment was marked as resolved.
This comment was marked as resolved.
getdave
added
[Block] Navigation
|
Pinging @andrewserong as I know he's been working a lot on list view. This PR fixes the issues but curious how you feel about (partially) exposing an API ( This is a form of IoC but I wanted to get some more opinions before this ends up landing. Update: @scruffian's makes a good point that this API is only exposed via a private mechanic and even then only to a specific render prop. |
scruffian
reviewed
May 19, 2023
scruffian
reviewed
May 19, 2023
scruffian
reviewed
May 19, 2023
| @@ -221,6 +227,8 @@ function ListViewComponent( | |||
| BlockSettingsMenu, | |||
| instanceId, | |||
| renderAdditionalBlockUI, | |||
| insertedBlock, | |||
| setInsertedBlock, | |||
There was a problem hiding this comment.
We are stuffing even more stuff into context. Is that a problem?
There was a problem hiding this comment.
It's always a potential problem for performance because if the context changes and components which consume it (or their children) will be re-rendered.
The setter is stable, but the insertedBlock will change.
However, pre-optimising context is challenging. Better to include in the memo and then optimise if/when List View becomes a problem.
scruffian
reviewed
May 19, 2023
scruffian
approved these changes
May 19, 2023
There was a problem hiding this comment.
This is looking good to me. From what I can see the setter is only exposed to renderAdditionalBlockUI , which is a private prop so I'm not concerned about that.
I think we added the concept of lastInsertedBlockClientId to the store for this PR. Should we also consider removing that?
Co-authored-by: Ben Dwyer <ben@scruffian.com>
jeryj
approved these changes
May 19, 2023
|
|
||
| const shouldShowLinkUIForBlock = | ||
| blockSupportsLinkUI && | ||
| ! blockHasExistingLinkValue && // don't re-show the Link UI if the block already has a link value. |
There was a problem hiding this comment.
Is this check necessary? If it only appears after a user has inserted the block, it should never have a link, right? If so, I think we can remove all the checks for if there's a link or not.
There was a problem hiding this comment.
Yeh you could be right there. Have you got time to test that and commit as it's nearly my EOD?
… URL from the appender insertion
There was a problem hiding this comment.
Thanks for the ping!
The idea of ensuring that both the appender and the additional UI both have access to the inserted block / setInsertedBlock state sounds good to me. And like @scruffian mentioned, if it's a private API where it's being exposed, taking an iterative approach to this also sounds good 👍
One question I have from looking this over is about the renderAdditionalBlockUI function, and its positional params. Will we expect to add more params to this function in the future, and is it always expected that renderAdditionalBlockUI will return a React component? If so, I'm wondering if instead of it having a function name, if we could treat is as a component, and then instead of using positional params, it would receive props. So, something like:
<AdditionalBlockUI
block={ block }
insertedBlock={ insertedBlock }
setInsertedBlock={ setInsertedBlock }
/>Apologies if I've missed some details on the benefits of using a function / callback approach instead!
To put the question another way: do we have an idea yet of what the final / stable shape of this API might look like? I know much of the recent efforts have been about removing the experimental off canvas editor component, which has been great to see. Now that that's happened, I'm curious if we can hone in on what a stable API shape might look like. There are a lot of moving parts in the ListView component, and it'd be good if we can try to stabilise and document some of these more nuanced areas if we can — if only so that we don't accidentally break anything in follow-up PRs.
I'll just CC @talldan, too, for visibility, too.
Nice work fixing up the inconsistencies! +1 to @ajlende's comments that it's great when fixing the root cause for something can close out multiple issues 🙂
Comment on lines
+71
to
+74
| renderAdditionalBlockUI( | ||
| block, | ||
| insertedBlock, | ||
| setInsertedBlock |
There was a problem hiding this comment.
This now forms part of the API of renderAdditionalBlockUI as positional params. If and when renderAdditionalBlockUI becomes part of the stable API, insertedBlock and setInsertedBlock might not be applicable for some use cases, but we also might want to add additional params at some stage. Would it be worth switching renderAdditionalBlockUI to use an object as its param?
There was a problem hiding this comment.
Thanks for the feedback on this. When I implemented this I also considered that but (for a reasons I can't recall) neglected to include it in the PR 😕
I think it's a good idea. The useful state that might need to be made available to this render prop in future might be many things so we should convert to object.
As for using a component - that seems logical to me. This PR didn't implement that but it's definitely worth calling out. It's very similar to the renderAppender arg of InnerBlocks so we could follow the pattern set down there.
There was a problem hiding this comment.
Nice. Thanks for opening up the follow-up issue!
This was referenced May 22, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Fixes inconsistent and unexpected appearance of Link UI within the Nav block list view editing. Also ensures the correct value appears in the UI when multiple Nav blocks that reference the same menu are in the template.
Also removes code that is no longer used.
Fixes #50601 and #50733
Why?
There's a long explanation of the problem(s) in the Issue.
TL;DR version
trunkthis relies on global state forlastInsertedBlockto find "the last block inserted into the editor" which seems logical, but in reality is not what we want to do.trunkthe Link UI will appear unexpectedly on "re-mount" if the last inserted block doesn't have aurl. Moreover, if there are multiple Nav blocks in the template referencing the same menu, then the Link UI will appear but with the "wrong" (unexpected) value - this value comes from another block on the page which is "syncing" with the menu being altered in the first blocks list view.How?
Moves all state about the "last inserted block" to rely on state that's local to the current list view. This is achieved by moving the getter/setter for "last inserted" within the List View and putting it within the
Contextobject of List View.As a result of utilising context, we are then able to pass the state about the "last inserted block" to the render prop callback which determines whether to display the Link UI.
Within the code in the Nav block which provides the render prop to the List View that is used in it's "list view editing mode", we then remove all global state and move to using the state provided to the render callback to determine whether to show the Link UI and also which block attributes should be shown in the Link UI.
The result of all this is that state about "last inserted" is made entirely local to the current List View instance and thus we avoid unexpected interactions that can come from consuming global editor state about "last inserted".
Testing Instructions
Test against the scenarios and testing instructions in both Issues - they should both no longer be in evidence:
- #50601
- #50733
Also please:
Our e2e tests for offcanvas provide some coverage but human trial/error is also valuable.
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2023-05-19.at.10-30-39.mp4