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

Nav Block - enable page creation from within Block #19775

Merged
merged 171 commits into from Feb 17, 2020

Conversation

@getdave
Copy link
Contributor

getdave commented Jan 21, 2020

Screenshot 2020-01-21 at 13 53 41

Closes #18900

This PR aims to allow users to create new Pages (only) on the fly from within the Navigation Block.

A Note on coupling with WordPress

Note that the LinkControl is within the Block Editor which is supposed to be "unaware" of WordPress. That said the existing implementation in master is already coupled to WordPress via URLInput which requests data from WP endpoints. Therefore we're following existing precedent here by introducing a prop to pass a handler function create entities on the fly. This is entirely optional and can be configured by the developer so it is less coupled to WP than you might imagine. There is a concept of "creating something" but that's it. We feel this is a good compromise.

Testing Instructions

  • Clear / delete your Posts/Pages.
  • Check out PR.
  • Create new Page or Post.
  • Insert Navigation Block.
  • Create a Page
    • start typing anything that could be a valid Page name into the search field
    • see suggestion to "Create Page"
    • click to select and create Page. Please note: Link UI will be auto-closed due to this intentional change.
    • when you reopen the Link UI you should see the Page has been created.
    • verify Page has been created under Dashboard > Pages

Other testing scenarios

  • Ensure that "Create" suggestion is not show when entering a URL.
  • Try "unusual" values in the input to try and break things (special chars...etc).
  • Create a user without permissions to create pages (contributor?). Check that the UI for creating a Page is not exposed to them within the Nav Block.

Todo

  • Consider using core/data to create the Pages rather than manual apiFetch.

  • Fix to ensure Create Page option is displayed if the entry contains spaces (eg: This is a Test).

  • Implement error handling for when Page cannot be created or times-out.

  • Implement handling when Page already exists. Perhaps when attempting to create a page which already exists, it seamlessly reuses the existing Page without any need for erroring. @marekhrabe and I decided that as per current WP convention a new page should be created with the entered title but with a different slug (eg: new-page-2).

  • Handle a11y concerns regarding the "Create" option not being a valid suggestion which you can move to via the Arrow keys.

  • Avoid showing the option to create pages when user doesn't have the capability (wp.data.select('core').canUser('create', 'pages'))

  • Ensure entity (aka Page creation) is possible using keyboard.

  • Implement checks against failure saveEntityRecord result types and also entry results which are missing appropriate values. This is needed in Nav Link Block edit createEntity() definition.

  • Avoid suggesting URLs for input that isn't a valid URL. This may be better handled in a separate PR as having both options isn't causing any regression. Waiting on #19871.

  • Add "Loading" spinner Icon to isResolving state when creating a Page (@jeryj ) - see #18900 (comment)

  • Ensure Error notice has correct spacing as per Design (@jeryj ).

  • Fix failing e2e tests due to use of uniqueId invalidating the snapshots. Check with @aduth that using uniqueId is a good idea. (@jeryj ).

  • Solve creation of unique IDs for the x2 manually created suggestion types.

  • Resolve suggestions label a11y issue (@jeryj).

  • Fix additional unwanted spacing below input when no search results. See here. (@jeryj)

  • Write e2e test for Nav Block to cover creating Pages from within Block.

  • Update color references to Sass vars in consultation with Designer (@jeryj )

  • Fix component possibly unmounted when state change occurs issue.

How has this been tested?

Unit tests. Manual tests.

Screenshots

Screen Capture on 2020-01-21 at 13-52-09

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 getdave requested a review from marekhrabe Jan 21, 2020
@getdave getdave added this to 👀 PRs to review in Navigation block via automation Jan 21, 2020
@getdave getdave changed the title Nav Block - enable create page from within Block Nav Block - enable page creation from within Block Jan 21, 2020
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Jan 21, 2020

Question:

In this implementation you cannot use the arrow to move to the Create Page option because it is not one of the suggestions held within the state of URLInput. You can however use TAB to move to the Create Page item.

Is this valid a11y wise? My concern is that the Create Page option is contained within the listbox which means it probably should be selectable using the arrows?

If we want to promote the Create Page item to be a suggestion then it's going to be quite difficult. That said it might be possible.

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Jan 22, 2020

Note: in order to make this PR work we're going to need:

  • Ability to detect URLs vs Entity searches more reliably - here is a PR #19816 @aduth is working on a PR.
  • To promote the "Create" button to be a first-class suggestion return from fetchSearchSuggestions - without this it's not going to show up when there are no matching search results from entities API as dropdown only displays when there are valid suggestions in the state of URLInput.

Will tackle this tomorrow.

Copy link
Contributor Author

getdave left a comment

Noticed some inconsistencies and problems building up. We should look to tackle these before they become any more "baked in" @marekhrabe.

@getdave getdave force-pushed the add/create-new-page-within-nav-block branch from 37d322c to 49f0b2e Jan 30, 2020
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Jan 30, 2020

@aduth I rebased this branch and found a tonne of conflicts as a result of your refactoring of LinkControl.

If you have time to glance at the rebased file in this branch I'd feel a lot more confident knowing the rebase hadn't nuked any of your changes.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 31, 2020

I can plan to take a look in my morning. Are you otherwise expecting this to be review ready?

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Jan 31, 2020

I can plan to take a look in my morning.

Thanks (with caveat below).

Are you otherwise expecting this to be review ready?

No. Actually there is a bug whereby hitting ENTER on the "create" suggestion doesn't work. The form onSubmit isn't ever called as far as I can see.

Maybe best wait until that's fixed.

That said, weren't you looking at the handling of form onSubmit?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 31, 2020

What I was working on with regards to the submission is already merged, so if there's an issue, it's not one I'd be aware of or am actively working on.

See: #19651

@getdave getdave force-pushed the add/create-new-page-within-nav-block branch from d98b989 to 6882363 Feb 4, 2020
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Feb 17, 2020

Merge branch 'master' into add/create-new-page-within-nav-block

Did you do a merge here @jeryj?

ef0c09c

getdave added 10 commits Feb 17, 2020
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)
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Feb 17, 2020

Build failures are due to a known npm Issue/bug.

getdave added 4 commits Feb 17, 2020
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)
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Feb 17, 2020

I will merge this shortly following 👍 from @obenland.

Copy link
Member

obenland left a comment

Did a final smoke test with admin, editor, and author roles and everything works as expected.
Really nice work @getdave, thank you for your persistence! Also thanks to @marekhrabe, @jeryj, and @aduth for your tremendous help.

:shipit:

@getdave getdave merged commit 2aaa955 into master Feb 17, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Feb 17, 2020
@getdave getdave deleted the add/create-new-page-within-nav-block branch Feb 17, 2020
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Feb 17, 2020

Ok that's all folks. It's merged!

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.