Implement "Add block" UI for Nav block Link UI #57756
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
getdave
added
[Type] Enhancement
|
Size Change: +1 kB (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
FYI I know the visual styling is 100% here yet. |
If we do this, do we remove the behavior where the inserter functions like a normal inserter if you have other blocks in the navigation? I think yes. |
Difficult to say for sure how that might effect things. My instinct is that the current behaviour on |
getdave
commented
Jan 12, 2024
| @@ -89,7 +89,7 @@ export default function useFocusOutside( | |||
|
|
|||
| // Cancel blur checks on unmount. | |||
| useEffect( () => { | |||
| return () => cancelBlurCheck(); | |||
| //return () => cancelBlurCheck(); | |||
There was a problem hiding this comment.
@WordPress/gutenberg-components In this PR unfocusing the Popover by clicking elsewhere in the canvas causes the onClose callback supplied to Popover not to be called. This is because the timeout is cancelled by this effect when the component is unmounted.
However, if you click outside the canvas, such as on the List View, you will see that the timeout is called.
I do not currently understand and would appreciate any assistance you can offer to debug this.
All I want to do is reliably trigger the onClose callback when the Popover is closed.
There was a problem hiding this comment.
Here's a screencapture showing what I mean:
Screen.Capture.on.2024-01-12.at.13-55-37.mp4
There was a problem hiding this comment.
unfocusing the Popover by clicking elsewhere in the canvas causes the onClose callback supplied to Popover not to be called.
Mhh.. could that be related to the fact that, when clicking the canvas, the click is happening inside an iframe ?
Also cc'ing @stokesman who recently worked on related changes to the Modal component
There was a problem hiding this comment.
It's very odd. I thought it was something to do with that to because the set time out callback checks for document.focus() but that is not even being triggered because the timeout cancels before it can be triggered.
If I had to guess I'd say the effect runs the timeout cancelling code before the next tick of the event loop (i.e. when the timeout's callback would be executed).
There was a problem hiding this comment.
I tried restoring the return () => cancelBlurCheck(); line, and in my tests the popover is being closed when clicking on the canvas — I feel like I may be missing something on how to reproduce?
Re. the timeout callback not firing, another aspect that we should keep in mind is that, depending on the React tree, if the parent node of Popover gets unmounted, likely the timeout wouldn't have a chance to run — not sure if that's the case when canceling the addition of a new nav link.
There was a problem hiding this comment.
It's not as easy — basically what we've been so far is the first step towards implementing "modality" (see this great article for reference).
Doing it right is tricky, especially with a component like Popover. Ideally, at some point, we'd like to write a new version of Popover using ariakit which would natively support modality. In the past few months we've been experimenting with rewriting a few components and I've been recently working with @mirka to standardise such process.
Finally, a word of caution regarding modality in general, but also the current "hack": all pointer events (clicks, hovers, etc) will be disabled while that popover is open. For example, to select another navigation link, the user will need two clicks instead of one — so this solution is not ideal.
The "right" solution, IMO, would be to understand how to allow interactions with iframes to work as expected and trigger the close button. I know that the editor's iFrame already tries to do some custom event forwarding / bubbling, but I'm not familiar with it (cc @andrewserong who, if I'm not wrong, was also dealing with something related when working on drag and drop)
There was a problem hiding this comment.
I wonder if @ellatrix has some insight as well given her work on the iframed canvas?
There was a problem hiding this comment.
@getdave, I'm worried adding a div to close the popover is going to make this issue harder to unravel in the future. In the video you mention that if you click a link within the navigation, it does fire the callback. Does it also fire the callback if you click on another block or paragraph content?
There was a problem hiding this comment.
Created a followup here: #58825
There was a problem hiding this comment.
We should not bubble more events from the iframe, this is a hack that should be removed in the future.
If you want to know if the user clicked in the canvas/iframe, you could check if focus moved onto the iframe? Normally if you click in the iframe, focus will also move from the previous element to the iframe in the parent document.
Agreed. The behavior that the nav block inserter functions differently if there are non-link blocks in it. |
|
@jeryj To advance herre this PR will need a review and also a11y testing. The act of transitioning between the two views probably needs to be announced as the context switch is quite pronounced. |
This was referenced Jan 23, 2024
getdave
requested review from
dmsnell,
spacedmonkey,
noahtallen,
ObliviousHarmony,
t-hamano,
fluiddot,
dcalhoun and
gziolo
as code owners
scruffian
force-pushed
the
add/add-block-control-to-nav-block-link-ui
branch
from
a2d0a97
to
c029c9a
Compare
This was referenced Feb 8, 2024
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?
Adds a new "Add Block" control to the Link UI which is shown when adding links in the Navigation block.
Closes #50888
Closes #47063
Co-authored-by: getdave [email protected]
Co-authored-by: scruffian [email protected]
Co-authored-by: ciampo [email protected]
Co-authored-by: stokesman [email protected]
Co-authored-by: draganescu [email protected]
Co-authored-by: richtabor [email protected]
Co-authored-by: SaxonF [email protected]
Why?
Currently when the user clicks the plus icon within the Nav block, it defaults to auto-adding custom link variant of the
core/navigation-linkblock. However this makes it hard to add other types of blocks such as search, or other variants such as Page...etcThis PR improves the UX by offering an
Add blockcontrol add the bottom of the Link UI. When clicked the Link UI disappears and the block "Quick Inserter" is displayed with all the blocks that are allowed to be inserted.If the user selects a block then it is inserted. If they click back then the Link UI is displayed again.
How?
QuickInsertercomponent from Block Editor package (we might want to make this private for now)Testing Instructions
+to add a new block.Add blockshown add bottom of Link UI.Add blockBackbutton works.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-01-11.at.10-56-17.mp4