Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Nav Block - enable page creation from within the Block (#19775)
* Adds new create component to search results * Styling and i18n * Display approximation of Page slug to be created * Fix to ensure border above create button is displayed * Implement prototype async load of created Page * Add state and UI to represent link being in a resolving/loading state 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. * add basic API integration (inline in component) * actually create pages, not posts * Add API to control display of the Create functionality on LinkControl * Adds initial tests for Create button feature * Add more test use case data * extract API call to prop function * swap api fetch with dispatch * Improve a11y of label * Adds coverage for creation of Link and update of “Selected” UI * Adds tests to cover scenarios where Create Page option should NOT be shown * Fix tests to avoid edge cases * Fix test to expose bug in implementation 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. * Fix bug introduced in tests * Hide Create option if suggestion is a single Direct (URL) result only. This is becase we shouldn’t create pages from anything that looks like a Direct Entry URL. * remove temporary loading state * Promote Create button to top level suggestion. 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. * Reinstate full test conditions Left some commented out test conditions. These have revealed that the previous commit did not fix the outstanding broken tests. * Fix bug whereby editing state was no disabled follow successful onChange * Allow create button to display for initial suggestions * Improve test code comments * Tweaks to code layout and formatting * Generalise prop to enable on the fly creation of entities * Improve code comments around promoting CREATE option to a first class 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. * support other post types in create function * Use generated Ids for `id` prop of faux suggestions rather than ever decrementing static negative numbers * Remove Create option from initial suggestions Addresses #18900 (comment) If we need to reinstate this functionality we can simply revert this commit. * Fix broken tests due to incorrect faux API response mock format * Fix test by relaxing type checking on falsey value * Catch invalid saveEntityRecord response types and throw error 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`. * Add Error handling to LinkControl for Creating Entities If `createEntities` promise is rejected then the erorr is caught and handled and an appropriate error notice is shown to the user. * Fix reset button positioning when error Notice is displayed above search input * Force repopulation of search results on create button failure. * Avoid showing Create UI when there is not input value * Moves error note below search input UI * Remove forceUpdate This doesn’t actually work. Removing. * Refactor out a handle create function * Extract common handler for selecting suggestion. * Add test (failing) to cover creation of entities via keyboard * Resolve rebase errors * Removes unused keypress handler. 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. * Remove redundant onKeyDown prop Again onKeyDown prop is not used within `URLInput`. Nor is it spread from props. Never called in tests. Not called when testing manually either. * Improve naming of handler functions to better reflect purpose * Fix to allow keyboard and form submit to handle entity creation 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. * Tweak to error positioning styling * Update to ensure created Page entities conform to object schema required by LinkControl * Include test for loading state in create entity tests * Fix errors introduced during rebase * Fix implementation broken due to rebase. * Add i18n for Loading state * Ensure correct timing of stopEditing by awaiting entity creation promise * Fixes render conditionals to be mutually exclusive 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. * Fix broken conditional exposed by test failures. * Fix to stop users without create pages permission seeing the Create entity UI * Add test to verify Create UI not shown if valid handler not provided to component * Renamed 'Loading...' to 'Creating Page' to match mockup * Adds the <Spinner> component to the Creating Page loading state * Reduced padding on bottom of LinkControl list results and moved error message up to match design specs more closely. * Only add separator when it needs separating We only need a separator when there are other suggestions the Create button needs separation from. * Committed formatted js * Fixed broken unit test for checking 'Loading' string instead of updated 'Creating' string * Removed uniqueID from Manual URLs in Navigation Link results from Link 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 * Remove unrequired eslint disable rule Addresses #19775 (comment) * Update packages/block-editor/src/components/link-control/index.js Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com> * Rename `errorMsg` prop to be unabbreviated for clarity. * Remove unnecessary cast to Boolean Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com> * Update to utilise `rendered` title value. Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com> * Remove redundant prop usage * Restore commented out test This should never have been committed with commented out code. * Update to invert create entity “type” dependency 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). * Removes outdated `showCreateEntity` prop * Fix so that CREATE option not displayed if result is a direct entry URL Also amends tests which were incorrectly asserting. * Fix typo * Adds docblock to newly extracted function for clarity * Corrects comment which implied a specific visual layout and DOM order Addresses #19775 (comment) * Fix to use standardised “gray” var reference Addresses #19775 (comment) * Correct comment to reference the correct property. Addresses #19775 (comment) * Remove partially applied function as not necessary Addresses * #19775 (comment) * #19775 (comment) * Reinstate descriptive function name Addresses #19775 (comment) * Rename `createEntity` prop to `createSuggestion` Addresses #19775 (comment) * Attempt update of type definitions #19775 (comment) * Avoid need to use entity term where making it more generic can avoid this. * Remove unused timeout HOC * Fix misnamed property * Update Nav Block create handler to use term “Page” to better reflect purpose * Refactor handleCreatePage to async/await to avoid nesting Addresses #19775 (comment) * Set url items from LinkControl to have sanitized url as the id and updated 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. * Reinstate keyboard handling of suggestion selection bug fix. 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. * Update comment to better reflect need for `title` and `url` props to reflect input text value * Remove unwanted reference to entity type. The more generic we keep the errors the simpler this component needs to be. Addresses #19775 (comment) * Corrects usage of aria-label and aria-labelled by on the link control search results Use aria-labelledby to reference the visible label ID Use aria-label when no visible label * Attempt to fix Typescript docblock alignment * Updated hardcoded color values to scss color variables * add e2e test for creating page from nav * accomodate for self-closing link popover after selecting link (prevents react error) * update comment to match expected test result * Fix to cancel pending promises on unmount to avoid state updates Addresses #19775 (comment) * Make arbitary CSS value correspond to standardised var Addresses #19775 (comment) * Removes manual aria live in favour of speak and @wordpress/a11y Addresses #19775 (comment) * Remove unwanted whitespace. Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com> * Rename var to lowercase Addresses #19775 (comment) * Avoid unwanted arial-label* roles for visually hidden elements `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. * Move cancellable refs to component scope. Partially addresses #19775 (comment) * Lightened border color between link control suggestions and create new page button * Changed Create new Page text to New Page to match design * Added <mark> to New page title when creating a new page in the link control * Make page title of new page creation darker * Adjusted positioning on the create new page button padding and centering * Added clarifying comment on negative margin CSS * Correct tests to select by aria-label now aria-labelledby is removed. * Correct test to look for new Create button wording Changed due to Design request to change text but test was not updated. * Fix to use cancelable async handler and call stopEditing 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. * Refactor select handlers to use async/await * Move function invocation within try/catch boundary * Remove test spying on speak HOC as this isn’t possible via `LinkControl` directly. * Remove superflous visually hidden text in favour of aria-label Addresses #19775 (comment) * Fix space vs tabs linting issue * Correct spelling typos * Correct prop rename error from rebase * Fix missing references * Remove duplicate render of LinkControlSearchInput introduced during rebase * Removes unnecessary closing of Link UI 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. * Adds documentation for createSuggestion prop * Restore Link Settings to be always visable as per 6c591f4 * Update e2e snapshot * Revert "Update e2e snapshot" This reverts commit 98f7e9e. * Wait for rich-text to be focused instead of waiting for link control 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. * Added a check on the active element to make sure the focused block matches our newly created page title * Update test to target specific input field for focus state rather than 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. * Refactor e2e test to be absolutelty 100% sure we’re in the input before typing Previously the tests were less than strict about whether the focus was in the input element. Improve this across all tests. * Attempt to fix e2e test via simplifying selection of create button * Ensure Create button is “in view” by removing other results. 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. * Provide more context on why we mock search and create API requests in e2e test * Updated routes on mock responses in e2e navigation tests and snapshots to hopefully make e2e tests easier to debug * Added description and documentation to updateActiveNavigatinoLink * More explicity select which suggestion we want in updateActiveNavigationLink * Await input focus before selecting the label on updateActiveNavigationLink * Fixed xpath selector in updateActiveNavigationLink and made the search page term more unique * Remove duplicate error notice * Remove superflous prop Introduced via rebase (again). * Simply listbox labelling for a11y 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. * Remove manual speak of Notice as it’s now built in to the component Addresses #15745 * Remove requirement to pass a Promise for `createSuggestion` prop Addresses #19775 (comment) * Handle cancellable handler props using refs 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. * Fix Promise flow so that stopEditing is not called on createSuggestion 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. * Fix rebase regressions. * Remove cleanForSlug 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) * Removes `id` prop from “Create” result and uses static string for React key prop. See #19775 (comment) * Fix e2e test snapshot to account for using full URL as the suggestion prop without cleanForSlug * Remove explicit “politeness” and use default settings See #19775 (review) * Update docs to distinguish suggestion from value. * Adds comment for “Create” constant Addresses #19775 (comment) * Adds type def for suggestion and reformats type defs Addresses #19775 (comment) * Fix to not render create button if there is no search term. 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) * Reinstate new line to separate imports from code body. Addresses #19775 (comment) * Removes unecessary response checking on advice from @aduth See * #19775 (comment) * #19775 (comment) * Remove stray unwanted comments from tests Addresses #19775 (comment) * Update novmenclature to confirm to suggestion as opposed to entity * Remove redundant portion of conditional Addresses #19775 (comment) * Improves method name and supporting comments and doc block. Addresses #19775 (comment) * Refactor search results label to utilise VisuallyHidden component Addresses #19775 (comment) * Consolidate createSuggestion handling logic within single method 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) * Add punctuation to comments Addresses #19775 (comment) * Update error handling to include the message prop of the Error object if provided. Addresses #19775 (comment) * Fix broken comment Co-authored-by: Marek Hrabe <marekhrabe@me.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> Co-authored-by: Konstantin Obenland <obenland@automattic.com> Co-authored-by: Andrew Duthie <andrew@andrewduthie.com>
- Loading branch information
5 people
committed
Feb 17, 2020
1 parent
6f55017
commit 2aaa955