Nav Block - enable page creation from within Block #19775

This is require to accommodate the new async mode for setting a link. For example when a Page is being created async with the API.

…shown

Previously both tests would pass even though when testing in browser (using real API requests) the one with spaces would fail. This is because the mocked API always results results which ensures that suggestinos are always shown. However, if the API doesn’t return any results then URLInput has no suggestions state and so the suggestions dropdown doesn’t display which causes the Create Page option also not display. In fact it should display.

This is becase we shouldn’t create pages from anything that looks like a Direct Entry URL.

Previously the Create button was not part of the proper suggestions state. This caused several issues: 1. The Create option was only shown if there were suggestions (not what we want in certain circumstances) 2. It wasn’t possible to use keyboard to move to the Create button. 3. a11y concerns regarding having Create option outside of the true suggestions. We now make the searchSuggestions handler always append a special suggestion to the result set for create. This is a reserved suggestion which is only displayed in the UI when the appropriate conditions are met. Also fixes bug with un-needed call to `setIsEditingLink( false )` in async `onChange` handler causes invalid test failures.

Left some commented out test conditions. These have revealed that the previous commit did not fix the outstanding broken tests.

… suggestion This code is particularly ambiguous. Adding a clear comment here should remove a lot of confusion as to why this particular work around is required. It is far from ideal but without decoupling `LinkContorl` from `URLInput` it is necessary.

…decrementing static negative numbers

Addresses #18900 (comment) If we need to reinstate this functionality we can simply revert this commit.

Throwing here causes a rejected Promise to be returned from `createEntity`. This is then caught and handled inside `LinkControl` in order to centralise error handling. This area will need additional safeguards adding to catch response types which are errors or which don’t confirm to the expected format of `entity`.

If `createEntities` promise is rejected then the erorr is caught and handled and an appropriate error notice is shown to the user.

…rch input

This doesn’t actually work. Removing.

This appears never to be called. The prop is not used directly within `URLInput` or “spread” from the passed props. Testing by adding console.log and running the tests confirms that the code is not being run. Manual testing also.

Again onKeyDown prop is not used within `URLInput`. Nor is it spread from props. Never called in tests. Not called when testing manually either.

Previously only mouse click on the “create” suggestion correctly created a new entity. Improved by ensuring the form `onSubmit` handler also correctly triggers the onCreate handler if the suggestion type is the “create” type.

…red by LinkControl

Rebasing with master caused conditionals to be normalised into one. Now we have `isResolving` as state we need to ensure the component renders differently with this conditional involved. The clearest means of doing this is to break the render up into exclusive sections based on conditions.

…ntity UI

…to component

… message up to match design specs more closely.

We only need a separator when there are other suggestions the Create button needs separation from.

…ed 'Creating' string

…k Control Based on discussion about how to handle link results, I've removed the uniqueId as we may not need an ID at all for manual link results. Other possibilities are to include the ID property again and either set it to null or to the URL value

Addresses #19775 (comment)

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

This should never have been committed with commented out code.

Previously `LInkControl` was specifiying the type of `page`. This meant it was “aware” of the entity being created. This commit inverts that so that the consuming component is now required to define the type itself (this has been updated on Nav Block).

Also amends tests which were incorrectly asserting.

Addresses #19775 (comment)

Addresses * #19775 (comment) * #19775 (comment)

Addresses #19775 (comment)

#19775 (comment)

…this.

…purpose

Addresses #19775 (comment)

…dated e2e snapshot Based on conversations around potential issues with not including an id on navigation links, the ID has been added back in as the sanitized url as it should be fairly unique and not actually matter if it's unique.

Failing to pass the current input value as `url` of the suggestion causes the keyboard handling to break when hitting `ENTER`. Not entirely sure why at this stage.

…reflect input text value

The more generic we keep the errors the simpler this component needs to be. Addresses #19775 (comment)

… search results Use aria-labelledby to reference the visible label ID Use aria-label when no visible label

…ts react error)

Addresses #19775 (comment)

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

Addresses #19775 (comment)

`aria-labelledby` is only suitable when the referenced element is visible. `aria-label` should not be used to duplicate content already accessible within the element.

Partially addresses #19775 (comment)

…w page button

…ontrol

Changed due to Design request to change text but test was not updated.

Previously only the keyboard version of handleCreate was cancellable and treated as async. Now the onClick mouse version is also cancellable. Moreover we call stopEditing() correctly on resolve.

…ol` directly.

Addresses #19775 (comment)

…ebase

Calling setIsLinkOpen in the useEffect was causing setIsLinkOpen to be called twice. The act of moving focus back to the label is enough to trigger setIsLinkOpen to be called by the `Popover` onClose handler. Trying to call it again in the effect caused an error to be thrown regarding setting state on the unmounted component.

This reverts commit 98f7e9e.

…to disappear in e2e test We can't rely on the .block-editor-link-control__search-results-wrapper to be hidden, as there's a quick loading state between when that disappears and the link is focused. Waiting for the link to be focused allows this test to be passed while also checking that focus is placed correctly.

…tches our newly created page title

…n wrapper Previously the test was checking for focus on a <div>. Amended to target the <input /> element as that is what actually will receive focus.

…re typing Previously the tests were less than strict about whether the focus was in the input element. Improve this across all tests.

It’s possible that on certain screen sizes there is not enough room to display the Create button without having to scroll the LinkContorl seaerch results panel. This could cause the selection of the button to fail. Testing if this fixes the broken e2e tests or at least makes them more resilient.

… e2e test

…s to hopefully make e2e tests easier to debug

…ionLink

…nLink

…h page term more unique

Introduced via rebase (again).

As per this thread in WPOrg Slack (https://wordpress.slack.com/archives/C02RP4X03/p1581500184181100) it’s better to have a HTML based label over aria labels under all circumstances. Therefore despite what it says on MDN docs for listbox (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/listbox_role) we simplify to only use a HTML label and refer to that with aria-labelledby.

Addresses #15745

Addresses #19775 (comment)

Addresses #19775 (comment). Previously we were treating a functional component as though it would live forever when in fact it could easily be unmounted and all internal var references wiped. Using refs solves this as they persiste between renders.

…n error and error message is shown There were several problems here. * Originally the handleCreate method did `return` with a undefined value when the error was cancelled. This caused incorrect logic flows. * The `stopEditing` method was being called even if the Promise flow was handling an error state. This caused the “Currently selected” UI to show with an `undefined` value. * The `errorMessage` state was being reset ever time the input “changed”. This meant the error set in the Promise chain was being reset before it could be displayed. Fixing all the above issues has resolved the errors.

This was used to try and “clean” the id primarily to make it valid as a React key. However we’ve discovered that any string is potentially valid. Moreover cleanForSlug might end up making two urls that are otherwise distinct become identical via stripping out of various parts of the full URL. See #19775 (comment)

…ct key prop. See #19775 (comment)

… prop without cleanForSlug

See #19775 (review)

Addresses #19775 (comment)

This was a throw back to a previous state of the `LinkControl` component whereby we wanted to render a create option to allow the user to create blank pages. This was removed as a requirement but the component was not fixed to account for that. Addresses: #19775 (comment)

Addresses #19775 (comment)

See * #19775 (comment) * #19775 (comment)

Addresses #19775 (comment)

Previously the code to handle the async flow including error handling was duplicated across two handler props. Consolidating helps DRY things and avoid accidental errors being introduced. Addresses #19775 (comment)

Addresses #19775 (comment)

… if provided. Addresses #19775 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nav Block - enable page creation from within Block #19775

Nav Block - enable page creation from within Block #19775

Commits on Feb 14, 2020

Commits on Feb 17, 2020