NavigationMenu: <LinkControl /> integration. #18062

retrofox · 2019-10-22T15:04:29Z

Description

< LinkControl /> and <NavigationItemMenu /> integration.
This branch contains changes of both respective branches: ~~#17986~~ #17846

Screenshots

Types of changes

TODOs

Replace URLInput by LinkControl
Fix some of the linting errors
Store link value and settings in local state
When inserting a new menu item — or when the link attribute is empty in general — focusing on the text should open the link menu as the primary interaction.
Clicking "Change" actually removes the link, which is pretty destructive — pressing escape just leaves an empty link. I think "Change" should still have the field populated with the current URL.
When there is no placeholder you don't know if the RichText has focus.
After adding a link and clicking the label, the link editor disappears but then you have to click the link toolbar button twice to get the link editor back.

The following issues will be handled in a separated PR:

First time the link menu opens, the loading indicator is cut off:
It seems typing in the link field triggers "is typing", which feels a bit odd since the toolbar and movers disappear.
We should support Command/Ctrl+K to open the link flow.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

talldan · 2019-10-23T03:30:55Z

I notice this is a draft, so there's probably more to do, but I've given this a quick test to help identify those things. A few comments:

Looks like there needs to be a way to stop keyboard events from propagating outside the popover. An example is when pressing delete in the search input, it currently deletes the block. This should probably be rolled into Experimental Link creation interface #17846, so that it works out of the box. With the current link popover this code that stops propagation has to be added wherever its implemented—https://github.com/WordPress/gutenberg/blob/master/packages/format-library/src/link/inline.js#L106-L111
Searching for existing posts/pages doesn't seem to work.
When attempting to submit a url, there's an error TypeError: Cannot read property 'url' of undefined.

packages/block-editor/src/components/link-control/README.md

packages/block-library/src/navigation-menu-item/edit.js

talldan · 2019-10-25T05:03:25Z

@retrofox I've pushed a commit that brings this a bit closer. A summary of the changes:

Fix some of the linting errors
Store link value and settings in local state (a temporary measure until it can be stored as an attribute, but I think it's important to demonstrate the working link UI).
Removed handling for 'escape' key closing the LinkControl—this is handled internally in the Popover component, so not needed.
Remove autocompleteRef code. This is only needed when the suggestions are rendered in a popover, which isn't the case with this new UI.
Remove fetchSearchSuggestions data being passed into LinkControl as this is also handled internally in the component.

It mostly seems to work 👍. The main issue I'm seeing at the moment is an error when trying to select one of the suggestions using the keyboard (mouse works ok). I'm unsure if this is a problem only in this branch or if it needs to be fixed upstream in #17846. I think if that can be resolved and all the new components + props on existing components in #17846 marked as experimental it's pretty close (cc @getdave).

retrofox · 2019-10-25T12:44:23Z

Fix some of the linting errors
Store link value and settings in local state (a temporary measure until it can be stored as an attribute, but I think it's important to demonstrate the working link UI).
Removed handling for 'escape' key closing the LinkControl—this is handled internally in the Popover component, so not needed.
Remove autocompleteRef code. This is only needed when the suggestions are rendered in a popover, which isn't the case with this new UI.
Remove fetchSearchSuggestions data being passed into LinkControl as this is also handled internally in the component.

Thanks, @talldan 👍

retrofox · 2019-10-25T12:47:37Z

Remove fetchSearchSuggestions data being passed into LinkControl as this is also handled internally in the component.

Let me keep the searcher there just for dev purposes.

retrofox · 2019-10-28T22:19:00Z

I'd like to know your opinions about how to continue this implementation, which in short takes over of adding/editing a link in the Menu Item.

Points to discuss, relevant changes, suggestions, questions:

I've changed where the Popover anchors. In this PR (already merged) the popover is anchored at the link button. In this PR it anchors at the text field:

How the link can be cleaned? Should we add a clear button?
What should be the behavior when the user selects the link typing the ENTER key?
Right now, it selects the link from the suggestions but the popover keeps there. The way to dismiss it is either typing the ESC key or clicking outside.
Should it show an accept button?
Similar question when the user clicks on the suggested option.
In this PR, when the item defined a link it renders an ExternalLink when it isn't in the edition mode. Is it correct?
Should we add a has-link class (or something like that) in order to show the text underlined (or other styles) in the editing mode when the item has defined a link?

I've updated the gif in the PR description, just in case. Thanks in advance.

talldan · 2019-10-30T05:33:35Z

This will need a careful rebase now that #17846 is merged to master. Not sure what the best approach would be.

getdave · 2019-10-30T09:47:28Z

@retrofox Looks like there are a number of Design/UX related questions that need discussions and answers.

How the link can be cleaned? Should we add a clear button?

I discussed having the ability to remove/clear a link in the LinkControl with @shaunandrews. He was keen to avoid UI "bloat". That said it looks like this is a required feature @shaunandrews. What are your feelings on this? It could be an optional control which is only shown when a prop is provided to "show" it within the UI.

What should be the behaviour when the user selects the link typing the ENTER key?
Right now, it selects the link from the suggestions but the popover keeps there. The way to dismiss it is either typing the ESC key or clicking outside. Should it show an accept button?

Again, another real-world scenario reveals potential changes required to the LinkControl. I would say we need some kind of UI to allow a user to "commit" their selected link. Bear in mind we cannot immediately dismiss the Popover on the selection of a link because the user may need to toggle settings (eg: "Open in New Tab"). @shaunandrews is this something we add into LinkControl?

In this PR, when the item defined a link it renders an ExternalLink when it isn't in the edition mode. Is it correct?

This mirrors the existing implementations of a "link interface" within Gutenberg. The idea is that you don't click the "selected" link and get taken away from the Block Editor interface. I'd say it's correct and should remain "as is" unless there is a problem with it.

Should we add a has-link class (or something like that) in order to show the text underlined (or other styles) in the editing mode when the item has defined a link?

Doesn't seem like that could cause any harm and it could be useful in the future.

Addresses #18062 (comment)

open link control and show fake placeholder when it's a new iotem

…nent rendering

It adds a hacky solution to deal with both events that happen at the same time: LinkControl.onClose and ToolbatButton.onClick, removing the useState ussage in favor adding a setTimeout call.

Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>

Addresses #18062 (comment)

`fetchSearchSuggestions` seems to have been accidentally re-added. It’s not required. Addresses #18062 (comment)

talldan · 2019-11-06T09:39:50Z

Not sure why github added me as co-author to every commit just for rebasing 🤷‍♂

mtias · 2019-11-06T10:47:36Z

Great to see this in!

getdave · 2019-11-06T10:56:14Z

Great work on this @retrofox! 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

Thanks for all the help @talldan and @draganescu!

retrofox force-pushed the try/menu-item-with-link-ui branch 2 times, most recently from 934bda5 to 5e85b47 Compare October 22, 2019 19:59

retrofox changed the base branch from master to try/unified-link-ui October 22, 2019 20:00

retrofox changed the base branch from try/unified-link-ui to master October 22, 2019 20:00

retrofox force-pushed the try/menu-item-with-link-ui branch 2 times, most recently from 37b5e9c to c7b5364 Compare October 23, 2019 23:20

talldan reviewed Oct 24, 2019

View reviewed changes

packages/block-editor/src/components/link-control/README.md Outdated Show resolved Hide resolved

retrofox mentioned this pull request Oct 24, 2019

Experimental Link creation interface #17846

Merged

40 tasks

retrofox force-pushed the try/menu-item-with-link-ui branch from c7b5364 to f3cc035 Compare October 24, 2019 13:37

talldan reviewed Oct 25, 2019

View reviewed changes

packages/block-library/src/navigation-menu-item/edit.js Outdated Show resolved Hide resolved

retrofox force-pushed the try/menu-item-with-link-ui branch 2 times, most recently from 1ce56b5 to b438028 Compare October 25, 2019 14:17

retrofox mentioned this pull request Oct 28, 2019

Mark Link Creation Interface as Experimental #18110

Merged

retrofox force-pushed the try/menu-item-with-link-ui branch 6 times, most recently from f31293f to a88b994 Compare October 28, 2019 20:23

retrofox mentioned this pull request Oct 29, 2019

Link interface iteration #17557

Closed

retrofox force-pushed the try/menu-item-with-link-ui branch 3 times, most recently from c858e3b to 275a43c Compare October 29, 2019 20:11

getdave and others added 22 commits November 6, 2019 15:48

Update attribute name

9a2d2fb

Addresses #18062 (comment)

Fix save output to handle new tab attribute

c712717

Addresses #18062 (comment)

Update to simplify link update with default args

56536a1

Addresses #18062 (comment)

Remove content accidentally re-added in merge of rebase

0c965d8

navigation-menu-item: set title as label when it's empty

80cb5e8

navigation-menu-item: open LinkControl when new item

b9d2195

navigation-menu-item: new item workflow

293bfb3

open link control and show fake placeholder when it's a new iotem

navigation-menu: rename state hook

f6ebb92

navigation-menu-item: use camelCase for link ID

3b4a3ed

navigation-menu-item: set edit mode style

468eff8

navigation-menu-item: remove fake placeholder stuff

e85cf06

navigation-menu-item: no use effect to Initialize is open state

7044c24

navigation-menu-item: remove unneeded setting state

2dfa557

navigation-menu-item: move link control callback outside of the compo…

c063034

…nent rendering

navigation-menu-item: simplify closing linkcontrol

e1b64e4

It adds a hacky solution to deal with both events that happen at the same time: LinkControl.onClose and ToolbatButton.onClick, removing the useState ussage in favor adding a setTimeout call.

navigation-menu: fixing eslint errors

b77c663

Update packages/block-library/src/navigation-menu-item/edit.js

b4ca8ca

Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>

Update packages/block-library/src/navigation-menu-item/edit.js

cf3d2c8

Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>

Fixes bug where Link UI was visually divorced from target Menu Item

2c7b597

Addresses #18062 (comment)

Updates to remove unwanted prop being passed to LinkControl

15bf1b2

`fetchSearchSuggestions` seems to have been accidentally re-added. It’s not required. Addresses #18062 (comment)

navigation-menu: remove destination attr

f60ae05

navigation-menu-item: cleanup timer once unmount

c34be01

talldan force-pushed the try/menu-item-with-link-ui branch from c0b82b5 to c34be01 Compare November 6, 2019 07:49

talldan merged commit b138818 into master Nov 6, 2019

talldan deleted the try/menu-item-with-link-ui branch November 6, 2019 08:22

youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NavigationMenu: <LinkControl /> integration. #18062

NavigationMenu: <LinkControl /> integration. #18062

retrofox commented Oct 22, 2019 •

edited

Loading

talldan commented Oct 23, 2019

talldan commented Oct 25, 2019

retrofox commented Oct 25, 2019 •

edited

Loading

retrofox commented Oct 25, 2019

retrofox commented Oct 28, 2019 •

edited

Loading

talldan commented Oct 30, 2019

getdave commented Oct 30, 2019

talldan commented Nov 6, 2019 •

edited

Loading

mtias commented Nov 6, 2019

getdave commented Nov 6, 2019

NavigationMenu: <LinkControl /> integration. #18062

NavigationMenu: <LinkControl /> integration. #18062

Conversation

retrofox commented Oct 22, 2019 • edited Loading

Description

Screenshots

Types of changes

TODOs

Checklist:

talldan commented Oct 23, 2019

talldan commented Oct 25, 2019

retrofox commented Oct 25, 2019 • edited Loading

retrofox commented Oct 25, 2019

retrofox commented Oct 28, 2019 • edited Loading

talldan commented Oct 30, 2019

getdave commented Oct 30, 2019

talldan commented Nov 6, 2019 • edited Loading

mtias commented Nov 6, 2019

getdave commented Nov 6, 2019

retrofox commented Oct 22, 2019 •

edited

Loading

retrofox commented Oct 25, 2019 •

edited

Loading

retrofox commented Oct 28, 2019 •

edited

Loading

talldan commented Nov 6, 2019 •

edited

Loading