Add Missing URL state to Navigation Link Block #28861
Conversation
|
@jasmussen @shaunandrews This does a little bit with highlighting a Link block that is missing the URL and popping up the input fields. A really rough sketch, but please let me know if it's on the right track. (Setting the URL and text may or may not work) |
|
Size Change: +6.4 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Excited about this one! There's not a ton to see yet:
One thing that I think is pretty key for the placeholder menu item, perhaps beyond opening the linkcontrol by default, is that the menu item label is not editable at all until both URL and Label have been filled out (or for Page link, a page has been chosen). That means the menu item is completely inert until configured. I suppose we can keep the RichText component and inert it using Can we remove the rich text part of the menu item, until configured? |
|
To Shaun's point, it could make sense to rewind this and make it even simpler:
Did I get that right, Shaun? |
|
Yes, exactly. |
|
Thanks! That's was the direction I've been heading in to see what we can do without overhauling too much, but that list works even better than where I was. |
|
OK, is this a bit closer to the interaction that we are trying to hit? Screen.Recording.2021-02-10.at.3.32.21.PM.mov |
|
That seems to be on the right path! You can't do anything until you select a link. Very nice. |
|
Really nice work. I think the next step is to polish the visuals a bit. Shaun did you want to try that dimmed gray background from your mockup? Happy to give that a stab and push CSS here. Question: from looking at your video, the linkcontrol popover appears top/left aligned from where you click, whereas I believe it's top/center aligned in the master branch. Is this intentional and/or easy to change? Bonus: can you make the linkcontrol use the That would make it have a black border like the other block UI. |
I think this is a visual artifact because I was using a small viewport to record the video. I just tested it fullscreen and I think it's aligning correctly:
If not, I'm not sure which part needs to move, maybe take a screenshot of the video? (That's also with |
|
@jasmussen do you want to take a stab at the visuals for the "Missing URL" state? |
|
I pushed a little polish:
It now has the default font and size, and a gray background reminiscent of the initial setup state. @shaunandrews will be asking about the spacing here:
That's the default spacing applied to
I did not find an easy way to make the gray background fit the available space. I'll tinker a bit more. |
|
Okay, got it working:
|
|
So, in #28440, Shaun designs a "active state" for the placeholder menu item being edited. Like so:
The inactive state has the gray background:
But we have no such active state:
Should we have one? Another thing. From looking at this (https://github.com/WordPress/gutenberg/pull/28861/files#diff-161a9737e181e56e07148b7eee4795f129aa821f73993a276cf1fd9c17941b70R388) there's a click handler to open the link dialog. But this handler doesn't appear to open if I set keyboard focus, and press "Enter", so we probably need a different handler that works with both mouse and keyboard:
Finally, there's a conversation on verbiage. In the example here, I have a Page link, a Post link, and a custom Link. You can see the differentiation in menu item:
In his mockups. Shaun showed different labels for each. George is it feasible, without a great deal of complexity, to show different placeholder text depending on the item type? So instead of "Missing URL", you'd get:
@shaunandrews I trust you to correct my reading of the above ☝️ |
|
Colorized the link control separator for when the style is "alternate":
|
georgeh
changed the title
| @@ -100,6 +100,9 @@ class URLInput extends Component { | |||
| } | |||
|
|
|||
| componentWillUnmount() { | |||
| if ( this.suggestionsRequest?.cancel ) { | |||
| this.suggestionsRequest.cancel(); | |||
| } | |||
There was a problem hiding this comment.
Found this during debugging, if you quickly toggle the URLInput before the fetch returns, it will attempt to call setState() on the unmounted component. This cancels the suggestionsRequest Promise when it unmounts to avoid the issue.
I can break it out to a separate PR if needed, the rest of the code doesn't trigger this anymore.
There was a problem hiding this comment.
Probably okay to sneak in, maybe just leave a quick note in the summary. eg "also fixes ..."
Minor: We can remove the if check using optional chaining.
this?.suggestionsRequest?.cancel?.()
|
@jasmussen I worked with @gwwar and we were able to get the chooser menu to trigger on keyboard navigation, add highlighting when the nav menu is not selected, and update the placeholder text. Take a look and let me know what you think. Screen.Recording.2021-02-16.at.11.56.28.AM.mov |
|
@shaunandrews Joen's AFK for a few days, if you have time I'd appreciate your thoughts. |
|
This is coming together nicely! Here's a few notes:
When I click the empty link block, the block itself is focused. Instead, I'd expect focus to move to the search field within the popover. Also, the link popover seems mis-aligned with the link block; I'd expect them to align to their left edges. -- When I use my keyboard to select the empty link block, the Link UI immediately opens. This feels wrong. Instead, I'd expect to have to hit -- Perhaps unrelated to this PR, but once I move my focus into the Link UI popover I have no way to close the popover. I expect that I could hit --
The empty state is a little weird in the horizontal variation of the Navigation block, where the width of the link block is much larger than the empty state shape. Perhaps this is expected — and if you put it in a Columns block, its feels much better — I just wanted to note the strangeness. |
|
I pushed a little fix to the border and the margin:
The focus ring is now correctly spaced all around the placeholder, and the border is inset so it has the same size as the gray solid blob. These two:
Great point.
Sorry if I misled on this before.
I'll take a look. |
that'll fix it — but I'm tempted to think we should fix this separately, because it's a general issue with the navigation block menu items:
And it does kind of open the question: what should the default behavior for that vertical version be? Happy to make a PR to add this separately, let me know! |
|
George, this PR is in good shape! It needs a rebase, and then Shaun's point about focus addressed, then we can land it. Great work! |
|
Right now I'm reworking how the focus events happen, which should address Shaun's points. What I'm trying to do is: Given the URL is not set:
Once that all works I'll get another review and get this merged |
Co-authored-by: Kerry Liu <[email protected]>
Note that we had to use pretty high specificity to override the text color Co-authored-by: Kerry Liu <[email protected]>
georgeh
force-pushed
the
update/nav-block-link-placeholder
branch
from
edd5786 to
93daed3
Compare
I mentioned the keyboard focus thing earlier, and can confirm the focus still sticks with the placeholder — I'd expect it to move into the popover and focus the search input. Related to this, once the popover does appear, its impossible to get rid of it without selecting another block. This is probably not a big deal, but as a keyboard user, I'd expect some way to get back to block itself; I instinctually try hitting I'd expect the placeholder state to work like a toggle. Click it once, and the popover opens and my focus moves to the search box. Click it a second time and the popover should close and my focus should move back to the block. -- Very minor, but I think there should be a |
|
@shaunandrews can you take another look? There was a regression when I rebased earlier. |
|
This is what I see:
It feels like it's perfect to me now! And it's a vast improvement over what we had before, night and day. This is great. |
There was a problem hiding this comment.
Due to the "is-alternate" feedback from Shaun's PR, let's also remove it here (see #28861 (comment)) — we can alway revisit the heuristic for when to show one or the other. So you can remove that, but otherwise, this is great.
If she has time, I'd also like one quick sanity check from @gwwar, but outside of that, this is delicious. Let's land it as soon as we can!
|
For folks reviewing adding a |
| @@ -100,6 +100,9 @@ class URLInput extends Component { | |||
| } | |||
|
|
|||
| componentWillUnmount() { | |||
| if ( this.suggestionsRequest?.cancel ) { | |||
| this.suggestionsRequest.cancel(); | |||
| } | |||
There was a problem hiding this comment.
Probably okay to sneak in, maybe just leave a quick note in the summary. eg "also fixes ..."
Minor: We can remove the if check using optional chaining.
this?.suggestionsRequest?.cancel?.()
|
TIL you can use Clear to merge? Edit: Confirmed with Joen that it's gtg |
Description
Changing the Navigation Link block to highlight when a link is missing
Closes #28440
Closes #23326
How has this been tested?
Screenshots
Screen.Recording.2021-02-16.at.11.56.28.AM.mov
Types of changes
When the URL is missing from a Navigation Link Block:
Bugfix:
URLInputwhen the component is unmounted to avoid callingsetStateon an unmounted componentChecklist:
I've included developer documentation if appropriate.I've updated all React Native files affected by any refactorings/renamings in this PR.