Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NavigationMenu: <LinkControl /> integration. #18062

Merged
merged 39 commits into from Nov 6, 2019

Conversation

@retrofox
Copy link
Contributor

retrofox commented Oct 22, 2019

Description

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

nav-menu-empty-link

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.
@retrofox retrofox force-pushed the try/menu-item-with-link-ui branch 2 times, most recently from 934bda5 to 5e85b47 Oct 22, 2019
@retrofox retrofox changed the base branch from master to try/unified-link-ui Oct 22, 2019
@retrofox retrofox changed the base branch from try/unified-link-ui to master Oct 22, 2019
@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Oct 23, 2019

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 #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.
@retrofox retrofox force-pushed the try/menu-item-with-link-ui branch 2 times, most recently from 37b5e9c to c7b5364 Oct 23, 2019
@retrofox retrofox mentioned this pull request Oct 24, 2019
37 of 40 tasks complete
@retrofox retrofox force-pushed the try/menu-item-with-link-ui branch from c7b5364 to f3cc035 Oct 24, 2019
@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Oct 25, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Oct 25, 2019

  • 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

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Oct 25, 2019

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 retrofox force-pushed the try/menu-item-with-link-ui branch 2 times, most recently from 1ce56b5 to b438028 Oct 25, 2019
@retrofox retrofox force-pushed the try/menu-item-with-link-ui branch 6 times, most recently from f31293f to a88b994 Oct 28, 2019
@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Oct 28, 2019

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.

@retrofox retrofox force-pushed the try/menu-item-with-link-ui branch 3 times, most recently from c858e3b to 275a43c Oct 29, 2019
@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Oct 30, 2019

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

@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Oct 30, 2019

@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.

getdave and others added 20 commits Oct 31, 2019
open link control and show fake placeholder when it's a new iotem
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>
Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>
`fetchSearchSuggestions` seems to have been accidentally re-added. It’s not required.

Addresses #18062 (comment)
@talldan talldan force-pushed the try/menu-item-with-link-ui branch from c0b82b5 to c34be01 Nov 6, 2019
@talldan talldan merged commit b138818 into master Nov 6, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Canceled
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Nov 6, 2019
@talldan talldan deleted the try/menu-item-with-link-ui branch Nov 6, 2019
@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Nov 6, 2019

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

@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Nov 6, 2019

Great to see this in!

@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 6, 2019

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

Thanks for all the help @talldan and @draganescu!

@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
* 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 <daniel.richards@automattic.com>

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

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

* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.