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 <[email protected]>

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!

@getdave

* link-control: doc * navigation-menu-item: replace URLPopover by LinkControl * navigation-menu: do not set link when onClose * navigation-menu-item: ensuring hide popover once it closes * navigation-menu-item: anchor LinkControl at TextControl * navigation-menu-item: render external link * navigation-menu-item: remove link object from attrs * navigation-menu-item: handing close link popover * navigation-menu: apply colors correctly in edition mode * navigation-menu: set text color to external link * navigation-menu-item: apply text color to external link * fixing eslint errors/warnings * fix camelcase warning flawlessly props to @getdave * navigation-menu-item: set current link as null when it changes * link-control: doc fixes * Updates setting change handler to test for specific setting and value Addresses WordPress#18062 (comment) * Remove hyperlinks from nav item labels Addresses WordPress#18062 (comment) * Update attribute name Addresses WordPress#18062 (comment) * Fix save output to handle new tab attribute Addresses WordPress#18062 (comment) * Update to simplify link update with default args Addresses WordPress#18062 (comment) * Remove content accidentally re-added in merge of rebase * navigation-menu-item: set title as label when it's empty * navigation-menu-item: open LinkControl when new item * navigation-menu-item: new item workflow open link control and show fake placeholder when it's a new iotem * navigation-menu: rename state hook * navigation-menu-item: use camelCase for link ID * navigation-menu-item: set edit mode style * navigation-menu-item: remove fake placeholder stuff * navigation-menu-item: no use effect to Initialize is open state * navigation-menu-item: remove unneeded setting state * navigation-menu-item: move link control callback outside of the component rendering * navigation-menu-item: simplify closing linkcontrol 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 * Update packages/block-library/src/navigation-menu-item/edit.js Co-Authored-By: Daniel Richards <[email protected]> * Update packages/block-library/src/navigation-menu-item/edit.js Co-Authored-By: Daniel Richards <[email protected]> * Fixes bug where Link UI was visually divorced from target Menu Item Addresses WordPress#18062 (comment) * Updates to remove unwanted prop being passed to LinkControl `fetchSearchSuggestions` seems to have been accidentally re-added. It’s not required. Addresses WordPress#18062 (comment) * navigation-menu: remove `destination` attr * navigation-menu-item: cleanup timer once unmount

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 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 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 21 commits November 6, 2019 15:48

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 <[email protected]>

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

cf3d2c8

Co-Authored-By: Daniel Richards <[email protected]>

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

Navigation editor automation moved this from 👀 PRs to review to ✅ Done 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

NavigationMenu: <LinkControl /> integration. #18062

NavigationMenu: <LinkControl /> integration. #18062

retrofox commented Oct 22, 2019 •

edited

talldan commented Oct 23, 2019

talldan commented Oct 25, 2019

retrofox commented Oct 25, 2019 •

edited

retrofox commented Oct 25, 2019

retrofox commented Oct 28, 2019 •

edited

talldan commented Oct 30, 2019

getdave commented Oct 30, 2019

talldan commented Nov 6, 2019 •

edited

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

Description

Screenshots

Types of changes

TODOs

Checklist:

talldan commented Oct 23, 2019

talldan commented Oct 25, 2019

retrofox commented Oct 25, 2019 • edited

retrofox commented Oct 25, 2019

retrofox commented Oct 28, 2019 • edited

talldan commented Oct 30, 2019

getdave commented Oct 30, 2019

talldan commented Nov 6, 2019 • edited

mtias commented Nov 6, 2019

getdave commented Nov 6, 2019

retrofox commented Oct 22, 2019 •

edited

retrofox commented Oct 25, 2019 •

edited

retrofox commented Oct 28, 2019 •

edited

talldan commented Nov 6, 2019 •

edited