Move linkUI to own custom appender in navigation block #68109
+262
−240
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
|
Size Change: +34 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
jeryj
requested review from
ellatrix,
ajitbohra and
fabiankaegy
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @[email protected]. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jeryj
added
[Block] Navigation
getdave
reviewed
Dec 19, 2024
There was a problem hiding this comment.
Thanks so much for working on this.
I've been thinking about this effort and the key things we need to solve for her are:
- Avoiding focus return problems that were evidenced in the related Issues.
- Avoiding allowing the user to get into unusual states
In my testing we're not quite there on both of these points. I'm sure it's possible to fix but I noticed a couple of key problems.
Firstly, if the Nav block is focused and you click on the appender and then click Add block you are shown an unfiltered list of block types including Group, Image and even Navigation itself. This would be extremely confusing and needs to be resolved.
Screen.Capture.on.2024-12-19.at.09-26-06.mp4
Secondly, if you focus away of the Link UI at any point without completing the action you end up with an empty link in the block. For context, empty links like this were historically something that caused a lot of confusion for users and we need to be sure we're not regressing that behaviour (aside: we should add an e2e test to provide coverage against this type of regression). I think that's why we had a lot of the focus handling logic in this block in the first place which is regrettable as this is what this PR is attempting to remove.
Screen.Capture.on.2024-12-19.at.09-26-41.mp4
My only final concern would be to get some Design feedback on whether we want to deviate from the pattern of Click appender -> See blocks (or in the case of Navigation and Buttons, see a default block automatically added). I'm not sure I have any strong feelings on this but we should get a confidence check before we create a new UX pattern for this one block.
Thanks again for continuing to tackle this Issue 🙇
jeryj
force-pushed
the
update/navigation-custom-appender
branch
from
eb7e029 to
3d6c8a8
Compare
|
I love the approach here. I'm not sure if we need design feedback since the user experience isn't any different is it? |
Yeah, to a user, I think you're right. The experience on this PR is:
|
✅ Done |
|
Flaky tests detected in e47dec2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12437150525
|
ajlende
reviewed
Dec 20, 2024
| // Focus and select the label text. | ||
| selectLabelText(); | ||
| } | ||
| }, [] ); |
There was a problem hiding this comment.
I expect you want the dependencies to rerun this like it was before.
Suggested change
| }, [] ); | |
| }, [ isLabelURLLike, isNewLink ] ); |
There was a problem hiding this comment.
I only want this to run on mount.
jeryj
commented
Dec 21, 2024
| : false, | ||
| placeholder: showPlaceholder ? placeholder : undefined, | ||
| __experimentalCaptureToolbars: true, | ||
| __unstableDisableLayoutClassNames: true, | ||
| } | ||
| ); | ||
|
|
||
| return <div { ...innerBlocksProps } />; | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
This wrapper should get removed. It was added while exploring a previous route.
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?
When you click the Navigation Block appender, it automatically inserts a link. If you cancel the action (escape or close the modal without selecting a block), the automatically added link is removed and focus gets lost or yanked to the previous block. It's also jarring to click the appender, and then have a new thing inserted and also open a modal.
So, rather than insert something automatically and then replace it (especially in the add block flow), this uses the LinkUI as the appender. It saves a bit of code and the UX feels much cleaner. Also, there have been a lot of bugs due to the previous flow.
Why?
Code quality, UX, and save ourselves the headaches of trying to manually manage when to insert and remove.
How?
Create a new component,
NavigationLinkAppenderand use it to wrap the<LinkUI/>for inserting links. Then, use this on both the Navigation Block and Submenu blocks.Testing Instructions
Testing Instructions for Keyboard
Especially test the keyboard flow. Add links and press Escape from the modal. You'll quickly see why this PR makes the keyboard implementation a lot better. Focus gets moved back to the appender instead of to the previous block.
Screenshots or screencast
Before
Screen.Recording.2024-12-18.at.4.38.58.PM.mov
After
Screen.Recording.2024-12-18.at.4.37.10.PM.mov