Refactor LinkControl API by youknowriad · Pull Request #19396 · WordPress/gutenberg

youknowriad · 2020-01-03T10:22:46Z

Refactors the LinkControl component API to have a single value/onChange couple instead of separating currentLink from currentSettings.

youknowriad · 2020-01-03T11:05:43Z

packages/block-library/src/navigation-link/edit.js

-	 * It's a kind of hack to handle closing the LinkControl popover
-	 * clicking on the ToolbarButton link.
+	 * This hack shouldn't be necessary but due to a focus loss happening
+	 * when selecting a suggestion in the link popover, we force close on block unselection.


@aduth @gziolo not sure if you have an idea about this. Why there's a focus loss when we select a suggestion causing the onClose of the Popover to never be called if we click outside.

const [ isLinkOpen, setIsLinkOpen ] = useState( ! url && isSelected );

It feels like this effect exists only because isSelected is checked only on the initial render. Did you try to refactor code to use isLinkOpen && isSelected as a guard clause for the link popover?

It seems to work in my testing, but I might miss something:

{ isLinkOpen && isSelected && <LinkControl ... }

yes, this works but I think we should fix the root issue instead which is the focus loss.

I'll update the code to follow this suggestion in the meantime

getdave

I learnt a lot reading through this refactor. Thank you.

My main concern would be to test a lot against the Navigation Block. ~~I'm going to try this now just to make sure I don't see any further issues.~~ I gave this the once over and I didn't notice anything other than...

When you first create a Nav Block the link UI doesn't automatically open anymore. Now you are encouraged to type your link text first and explicitly click the hyperlink toolbar button to add your link. I argued against this approach in the initial Nav Block work because I felt that users would expect to all the link first and might not understand that the toolbar button was required to add the hyperlink.

I wonder whether the LinkUI dialogue should always remain open when the Nav Item is focused? That way:

it's clear that you alter the link text via direct manipulation
it's obvious how you add a hyperlin (you don't have to make an effort to "understand" the interface)

packages/block-editor/src/components/link-control/index.js

getdave · 2020-01-03T11:30:30Z

packages/block-editor/src/components/link-control/index.js

 				<div className="block-editor-link-control__search">

-					{ ( ! isEditingLink && currentLink ) && (
+					{ ( ! isEditingLink ) && (


If we don't have a link then we don't want to show this UI interface. Just curious as to why you feel it's safe to remove this now?

the boolean seemed explicit enough for me. If we do add the check for the value here, we should also add it to force showing the input if the value is empty which was not the case.

packages/block-editor/src/components/link-control/index.js

packages/block-editor/src/components/link-control/search-input.js

jorgefilipecosta

This PR seems to work well. I was also able to replicate the issue @getdave found: when we add a new navigation item, the UI for the link does not open right away as it did before.
We should also update the recently merged buttons block to use the new API.

jorgefilipecosta · 2020-01-03T14:04:57Z

packages/block-library/src/navigation-link/edit.js

+	const link = {
+		title: title ? unescape( title ) : '',
+		url,
+		newTab: opensInNewTab,


Would it make sense to name the property opensInNewTab like the attribute?

We can I just used the default setting value precedently used.

jorgefilipecosta · 2020-01-03T14:20:37Z

packages/block-library/src/navigation-link/edit.js

+							} = {} ) => setAttributes( {
+								title: escape( newTitle ),
+								url: newURL,
+								label: label || escape( newTitle ),


Would it make sense to escape the tile inside the LinkControl component?

probably, but let's keep this for a separate PR as this one is growing.

@youknowriad Do you need me to create an Issue for this?

Please yes :) thanks

youknowriad · 2020-01-03T14:46:06Z

I fixed the issues raised but there's still a conceptual/design issue here.

we want the LinkControl to be visible once we first add a block (navigation menu item or button)
we also want the LinkControl to close when it loses focus (since it's a popover).

These two conflict with each other in some situations so for me, we have two options:

Like currently implemented in the Button block: the LinkControl is closed when you add a button.
Like currently implemented in the NavigationMenu: the LinkControl is opened on add but it's also focused.

Thoughts? which one we go with?

getdave · 2020-01-03T15:38:59Z

the LinkControl is opened on add but it's also focused.

I vote for the LinkControl being open and focused.

jorgefilipecosta · 2020-01-03T19:38:26Z

Like currently implemented in the NavigationMenu: the LinkControl is opened on add but it's also focused.

I think this option may present some accessibility issues. When the block is added the user is inside a popover and can not tab away from the link input field. If we follow this option we need to make sure screen reader users are aware that the focus is inside a popover right after the block is inserted.

youknowriad · 2020-01-06T08:14:15Z

So I think we need consistency here between the two blocks. I'd like some design eyes first before choosing a direction cc @karmatosed @mapk @mtias (context #19396 (comment))

but since this PR keeps the same behavior in master, I think it's ready to land.

jorgefilipecosta

The buttons block and the navigation block worked without any regression on my tests 👍

aduth · 2020-01-06T15:17:51Z

packages/block-library/src/navigation-link/edit.js

+					{
+						/**
+						 * The  isSelected check shouldn't be necessary but due to a focus loss happening
+						 * when selecting a suggestion in the link popover, we force close on block unselection.
+						 */
+					}


Is this comment still valid? Looking at the conversation from #19396 (comment), I'm wondering if this might have since been updated? Otherwise, I don't immediately see the isSelected check that this is referring to.

yes, it's necessary but should moved (it's done by the hook now)

mapk · 2020-01-07T01:04:29Z

@youknowriad, thanks for bringing this together.

Thoughts? which one we go with?

It would be great to see a consistent behavior.

If we begin with LinkControl closed, the Navigation block would look like this:

This feels a bit awkward for a few reasons:

Tiny space to begin typing.
No placeholder text like the Title, Paragraph, or other blocks.
The + icon may be misleading because it calls my attention more.

Increasing the space here might help, but without any clear area to begin writing a menu item, it's difficult to understand what I'm supposed to do. Creating a menu item isn't like free writing, it's more like creating a label of sorts, so I would half expect to see an input field or something. For these reasons, this approach may not work.

If we begin with LinkControl open, the Buttons block would look like this:

This feels better between the two options. BUT when creating a button, I'd like to create the button text first and worry about where it goes second (I could be in minority here), especially in this case with a predefined button area that makes it very clear where to write. Thinking about this, that's what I'd like to do about the Navigation block too, but I don't think our UI makes that easy for the reasons stated above.

All this is to say, if we're driving for consistency, beginning with the LinkControl open feels like the better option.

youknowriad · 2020-01-07T08:59:01Z

Ok let's move forward with what we have then and reconsider if needed.

aduth · 2020-01-17T20:43:11Z

packages/block-editor/src/components/link-control/index.js

-	const handleDirectEntry = ( value ) => {
+	const handleDirectEntry = ( val ) => {


I guess this was changed to avoid shadowing with the top-level prop? I'd typically not ever consider an abbreviation to be an improvement. I might have suggested something like nextValue if that's what it's intended to represent in this context.

youknowriad marked this pull request as ready for review January 3, 2020 10:56

youknowriad requested review from Soean, ajitbohra, ellatrix, jorgefilipecosta and talldan as code owners January 3, 2020 10:56

youknowriad self-assigned this Jan 3, 2020

youknowriad added [Block] Navigation Affects the Navigation Block [Type] Code Quality Issues or PRs that relate to code quality labels Jan 3, 2020

youknowriad mentioned this pull request Jan 3, 2020

Add: Buttons block #17352

Merged

youknowriad commented Jan 3, 2020

View reviewed changes

getdave reviewed Jan 3, 2020

View reviewed changes

youknowriad added 5 commits January 3, 2020 13:56

Refactor LinkControl API

cd56b75

Fix typos

e4a6c23

Simplify popover closing/opening

b0befe3

Fix onKeyDown and onKeyPress on the root component

663f51f

Remove useless onModeChange callback

ca03d97

jorgefilipecosta force-pushed the refactor/link-control-api branch from 65c32c0 to ca03d97 Compare January 3, 2020 13:57

jorgefilipecosta reviewed Jan 3, 2020

View reviewed changes

youknowriad added 2 commits January 3, 2020 15:33

Fix new menu item issue

0b16c6c

Refactor the buttons component

45d1640

Fix opens in new tab for the buttons block

56aa64b

jorgefilipecosta approved these changes Jan 6, 2020

View reviewed changes

aduth reviewed Jan 6, 2020

View reviewed changes

Move comment to the right place

fff72bc

youknowriad merged commit 99da6f0 into master Jan 7, 2020

getdave mentioned this pull request Jan 8, 2020

NavigationLink: make the URL dynamic for links to Posts #18345

Closed

aduth reviewed Jan 17, 2020

View reviewed changes

ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020

mtias deleted the refactor/link-control-api branch June 8, 2020 13:37

		const handleDirectEntry = ( value ) => {
		const handleDirectEntry = ( val ) => {

Conversation

youknowriad commented Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

getdave left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Jan 3, 2020

Uh oh!

getdave commented Jan 3, 2020

Uh oh!

jorgefilipecosta commented Jan 3, 2020

Uh oh!

youknowriad commented Jan 6, 2020

Uh oh!

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapk commented Jan 7, 2020

Uh oh!

youknowriad commented Jan 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

youknowriad commented Jan 3, 2020 •

edited

Loading

getdave left a comment •

edited

Loading

youknowriad Jan 3, 2020 •

edited

Loading