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

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 added the [Block] Navigation Affects the Navigation Block label 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
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
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 Try/improve URI detection in <LinkControl /> #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 getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@getdave
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
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
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
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 Compare February 4, 2020 11:08
@getdave
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
Copy link
Contributor Author

getdave commented Feb 17, 2020

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

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)
@getdave
Copy link
Contributor Author

getdave commented Feb 17, 2020

I will merge this shortly following 👍 from @obenland.

Copy link
Member

@obenland obenland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@getdave getdave deleted the add/create-new-page-within-nav-block branch February 17, 2020 17:11
@getdave
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
Labels
[Block] Navigation Affects the Navigation Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation: Add option to create a new page
10 participants