Nav Block - show recent pages as default suggestions when creating Nav Links #19458

getdave · 2020-01-07T11:44:42Z

Description

When first creating a Nav Link ("item") Block the hyperlink UI presents a blank search field. To better serve the default use case of adding pages, this PR updates the implementation to display a list of the most recently created Pages.

This involves a refactor of the URLInput to add a new boolean prop initialSuggestions which (internally) ~~affords the ability to trigger rendering of search results without anything being entered into the <input> field~~ (for more on this see the original Issue) triggers a call for default search results when the input field is empty.

How has this been tested?

Add Nav Block
Insert new Nav Link Block.
See a list of most recently created Pages displayed as the default search results in the hyperlink creation UI.

Screencapture

Types of changes

New feature (non-breaking change which adds functionality).

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.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

getdave · 2020-01-07T14:46:36Z

Pinging @youknowriad because I know he's concerned about ongoing changes to LinkControl.

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

shaunandrews · 2020-01-07T18:58:03Z

I wonder if 5 items is too many. With the way things fade out, it looks a little awkward. Maybe 3 suggestions would help clean up the default view of this popover, making it visually simpler by default.

jeryj · 2020-01-07T21:51:24Z

I much prefer this implementation to the empty state. Nice work, @getdave! Here are the few things I found during my functional review. The first and second likely the only one that seems worth addressing within this PR.

Always update text from selected link?

When selecting a new suggestion off of a page that already has text, I think the text should default to being replaced by the selected title. Currently it remains the same. I can see how this would be maybe be preferable if they entered custom content, but I think it's probably more likely they just want the text replaced.

Keyboard navigation?

At the moment there is no way (that I saw) to access the pages list via the keyboard. The tab key cycles between the input and the X to clear the link.

First initialization of empty menu doesn't show the recommended links

I'm not sure what is causing this one, but the initialization of an empty menu on a new page doesn't show the pages list. Any subsequent ones show the list. If the page already has a nav block, it also shows the list fine. It appears to only occur on the first initialization and then the auto-opening of the link field.

Account for items already in the menu?

This isn't necessarily an issue, but do we want to handle the suggestions based on what items are already in the menu? The most recently published items are already within the menu in this screenshot.

getdave · 2020-01-08T11:36:12Z

I wonder if 5 items is too many. With the way things fade out, it looks a little awkward. Maybe 3 suggestions would help clean up the default view of this popover, making it visually simpler by default.

@shaunandrews I've updated to 3 as suggested (see updated screencapture in PR desc)

getdave · 2020-01-08T11:39:06Z

@jeryj Thanks for the review.

First initialization of empty menu doesn't show the recommended links

Nice catch. This was a bug in my implementation. I've resolved this and you can see it working in the screencapture in the PR desc.

Keyboard navigation?

Another great catch. I've also fixed this but I'd appreciate you confirming via a manual test. Thanks.

Always update text from selected link?

I think this is a good point but not for this PR. Would it be possible for you to raise an Issue and then we can discuss the desired behaviour?

Account for items already in the menu?

We don't but we could...I'm thinking maybe in a followup PR so as not to block the basic working implementation but I could be persuaded...

getdave · 2020-01-08T16:41:02Z

I've broken the tests. Am going to look at this tomorrow.

apeatling · 2020-01-08T19:00:09Z

I had a good test of this. I think having the recent pages there is a good addition. Plus one to @jeryj's feedback above.

I think this could merge as it exists because it works well, however I've opened another issue with some visual improvements that I think should be discussed and iterated on as a follow up:

#19513

As per the GH thread below, this refactors the code to avoid the need to introduce a new fetch API around `__experimentalInitialSuggestions`. Now instead we simply reuse the existing fetchSuggestions handler to get the initial results. The only different being we introduce an arguments object to queries to restrict the number of results displayed for initial Suggestions. See #19458 (comment)

Addresses #19458 (comment)

…uggestions on update or mount

This is a temp fix and I’ve raised an Issue to solve the core issue which will then make this fix redundant. #19634

getdave · 2020-01-15T12:07:26Z

I neglected to update the Nav Block to use the renamed showSuggestions prop. Created a followup PR here.

aduth · 2020-01-16T19:14:13Z

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

+	const { keyCode } = event;
+
 	event.stopPropagation();
+
+	if ( keyCode === ENTER ) {
+
+	}


Were these changes intended to have been included? Looks like dead code to me.

Dead code. Being removed elsewhere.

getdave added the [Block] Navigation Affects the Navigation Block label Jan 7, 2020

getdave self-assigned this Jan 7, 2020

getdave requested a review from youknowriad January 7, 2020 14:03

getdave marked this pull request as ready for review January 7, 2020 14:03

getdave requested review from ajitbohra, ellatrix, jorgefilipecosta, Soean and talldan as code owners January 7, 2020 14:03

getdave requested review from obenland and retrofox January 7, 2020 14:48

youknowriad reviewed Jan 7, 2020

View reviewed changes

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

getdave requested review from marekhrabe and frontdevde January 7, 2020 16:08

apeatling mentioned this pull request Jan 8, 2020

Improve context to initial suggestions on Nav Link Block #19513

Closed

This was referenced Jan 9, 2020

Refactor LinkControl to remove dependency on URLInput #19523

Closed

Refactor URLInput to remove requirement for Promise in fetchSuggestions #19526

Open

getdave requested review from nerrad and ntwb as code owners January 9, 2020 14:01

getdave added 17 commits January 15, 2020 09:40

Removes redundant const comment

8b23050

Test fix to e2e tests

7d34ed5

Try looser mock request matching to fix e2e test failures

c37497e

Try fix Travis e2e test failure by clearing request mocks

cf55b73

Fix not awaiting mock setup

573972f

Fix inadvertant revert of e2e test fix

ee6c5b8

Refactor to use empty value as condition for initialSuggestions request

5eef3dd

Addresses #19458 (comment)

Fix “suggestions” typo

f500fe4

Update to reuse existing edit constant

0208bc6

Fix initial suggestions showing when input has value

2244e33

Fix potential infinite render using flag to conditionalise updating s…

a31f651

…uggestions on update or mount

Add test to cover inifinite re-render loop.

3b8b6ea

Fixes typo in WordPress ORG link in e2e tests

1ca624b

Fix e2e test failure due to console warning in LinkControl

b6d6c26

This is a temp fix and I’ve raised an Issue to solve the core issue which will then make this fix redundant. #19634

Revise snapshots

abe616c

Restore missing fetchSearchSuggestions prop following rebase

259938a

getdave force-pushed the add/show-pages-default-when-creating-nav-links branch from f66fadc to 259938a Compare January 15, 2020 09:44

getdave mentioned this pull request Jan 15, 2020

Fix: Bug on URLInput & Intermitent end to end test on the buttons block #19490

Closed

getdave added 3 commits January 15, 2020 10:21

Fix lint indentation error

385d04f

Rename prop

5dc5716

Fix test broken due to prop rename

8c648a3

getdave merged commit b448074 into master Jan 15, 2020

getdave deleted the add/show-pages-default-when-creating-nav-links branch January 15, 2020 11:12

getdave mentioned this pull request Jan 15, 2020

Update Nav Block to use new showInitialSuggestions prop on LinkControl #19667

Merged

6 tasks

aduth reviewed Jan 16, 2020

View reviewed changes

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

aduth mentioned this pull request Jan 25, 2020

Block Editor: Consider events only from DOM descendents (WritingFlow, ObserveTyping) #19879

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nav Block - show recent pages as default suggestions when creating Nav Links #19458

Nav Block - show recent pages as default suggestions when creating Nav Links #19458

getdave commented Jan 7, 2020 •

edited

Loading

getdave commented Jan 7, 2020

shaunandrews commented Jan 7, 2020

jeryj commented Jan 7, 2020 •

edited

Loading

getdave commented Jan 8, 2020 •

edited

Loading

getdave commented Jan 8, 2020 •

edited

Loading

getdave commented Jan 8, 2020

apeatling commented Jan 8, 2020 •

edited

Loading

getdave commented Jan 15, 2020

aduth Jan 16, 2020

getdave Feb 6, 2020

Nav Block - show recent pages as default suggestions when creating Nav Links #19458

Nav Block - show recent pages as default suggestions when creating Nav Links #19458

Conversation

getdave commented Jan 7, 2020 • edited Loading

Description

How has this been tested?

Screencapture

Types of changes

Checklist:

getdave commented Jan 7, 2020

shaunandrews commented Jan 7, 2020

jeryj commented Jan 7, 2020 • edited Loading

Always update text from selected link?

Keyboard navigation?

First initialization of empty menu doesn't show the recommended links

Account for items already in the menu?

getdave commented Jan 8, 2020 • edited Loading

getdave commented Jan 8, 2020 • edited Loading

getdave commented Jan 8, 2020

apeatling commented Jan 8, 2020 • edited Loading

getdave commented Jan 15, 2020

aduth Jan 16, 2020

Choose a reason for hiding this comment

getdave Feb 6, 2020

Choose a reason for hiding this comment

getdave commented Jan 7, 2020 •

edited

Loading

jeryj commented Jan 7, 2020 •

edited

Loading

getdave commented Jan 8, 2020 •

edited

Loading

getdave commented Jan 8, 2020 •

edited

Loading

apeatling commented Jan 8, 2020 •

edited

Loading