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 - improving initial state by adding Block placeholder #18363

Merged
merged 17 commits into from Nov 11, 2019

Conversation

@getdave
Copy link
Contributor

getdave commented Nov 7, 2019

Closes #18307.

This PR aims to improve the initial state of the Nav Block by adding a placeholder that affords the choice to:

  1. Create a Menu from existing pages.
  2. Create an empty Menu from scratch.

In future, this will be augmented to enable the creation of Menu from an existing Menu (but that is for another day!).

Note: if the Block already has items within it (ie: you already created your menu) the placeholder will not show.

Questions

  • Would it be better to only request the Pages on demand rather than upfront? If so I'll need some advice on using @wordpress/core-data to achieve this as I can't seem to wrap my head around how I'd structure the code (cc @mtias ).

How has this been tested?

Manual testing.

Screenshots

Screen Capture on 2019-11-08 at 14-34-38

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.
@getdave getdave self-assigned this Nov 7, 2019
@getdave getdave added this to 👀 PRs to review in Navigation block via automation Nov 7, 2019
@getdave getdave marked this pull request as ready for review Nov 7, 2019
@getdave getdave requested review from draganescu, mtias and karmatosed Nov 7, 2019
@getdave getdave mentioned this pull request Nov 7, 2019
0 of 2 tasks complete
@getdave getdave changed the title Adds initial placeholder with basic state toggle Nav Block - add Block placeholder Nov 7, 2019
@gziolo gziolo mentioned this pull request Nov 7, 2019
3 of 9 tasks complete
Copy link
Contributor

mcsf left a comment

I think the question of state in #18363 (comment) is important. Non-attribute state should be minimised, and so far I'm not convinced we need any.

The pitfalls of working with augmented state are illustrated by all the unexpected places where it breaks:

gut-nav-menu

packages/block-library/src/navigation-menu/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/navigation-menu/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/navigation-menu/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/navigation-menu/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/navigation-menu/edit.js Outdated Show resolved Hide resolved
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Nov 7, 2019

One design feedback, could we reduce the spacing to be like other placeholders, please?

image

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 8, 2019

One design feedback, could we reduce the spacing to be like other placeholders, please?

@karmatosed Noted. Thanks to Dan we are now making use of more of the defaults provided by <Placeholder />. I've then adjusted some other spacing.

Please note that the buttons are spaced by 14px between them. This is:

  • 4px: space between the buttons on the Gallery Block placeholder.
  • 10px hoz padding value of a default button

I add the 10px because the 2nd button is in isLink style which means it doesn't have any inner padding so I compensate by adding more spacing otherwise it looks squashed visually.

Here's what it looks like up against Gallery:

Screen Shot 2019-11-08 at 11 24 27

If you'd like to adjust further please do let me know.

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 8, 2019

I'm going to work on the state issues now.

getdave added a commit that referenced this pull request Nov 8, 2019
… innerBlocks

After a long discussion with @mtias we realised that unlike other similar implementations (Media+Text and Columns) of a placeholder, this one couldn’t use the “Template Options” feature of <InnerBlocks> to achieve the custom design required. Instead it rendered a `<Placeholder />` directly based on conditions.

As a result, we now use the `replaceInnerBlocks` from `block-editor` to dispatch an update to innerBlocks manually when the placeholder buttons are clicked.

Addresses

* #18363 (comment)
* #18363 (comment)
* #18363 (comment)
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 8, 2019

The pitfalls of working with augmented state are illustrated by all the unexpected places where it breaks:

@mcsf This now looks fixed as a result of my updated implementation.

Screen Capture on 2019-11-08 at 14-38-41

@getdave getdave requested a review from mcsf Nov 8, 2019
getdave added 7 commits Nov 8, 2019
… innerBlocks

After a long discussion with @mtias we realised that unlike other similar implementations (Media+Text and Columns) of a placeholder, this one couldn’t use the “Template Options” feature of <InnerBlocks> to achieve the custom design required. Instead it rendered a `<Placeholder />` directly based on conditions.

As a result, we now use the `replaceInnerBlocks` from `block-editor` to dispatch an update to innerBlocks manually when the placeholder buttons are clicked.

Addresses

* #18363 (comment)
* #18363 (comment)
* #18363 (comment)
@getdave getdave force-pushed the try/nav-block-improve-initial-state branch from 9c2b488 to b287122 Nov 8, 2019
@getdave getdave requested review from mcsf and mtias Nov 8, 2019
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Nov 8, 2019

If you'd like to adjust further please do let me know.

No that looks fine to me now, thanks.

Copy link
Member

karmatosed left a comment

Based on the design and to get this v1 out, approving.

@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Nov 10, 2019

@getdave under certain conditions (load from scratch, add block, choose starting blank) you can get the "loading" message when choosing the empty option:

Screenshot 2019-11-10 at 17 54 18

@getdave getdave moved this from 👀 PRs to review to 💻 Issues in progress in Navigation block Nov 11, 2019
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 11, 2019

@getdave under certain conditions (load from scratch, add block, choose starting blank) you can get the "loading" message when choosing the empty option:

I've replicated this by

  1. Loading new blank Post
  2. Throttling network speed in devtools to Slow 3G
  3. Adding Nav Block and immediately selecting use empty menu.

I've now fixed this issue.

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 11, 2019

New Bug:

  1. Load a new blank Post
  2. Throttle network speed in devtools to Slow 3G.
  3. Add Nav Block and immediately select Create from all top pages.

If the Pages are yet to load then there is an error.


Update

This bug is now fixed.

getdave added 2 commits Nov 11, 2019
…re Page data is available

Previously we didn’t know when the pages query had resolved. Moreover, we were allowing the Inspector controls to create Menus from Pages before the data was available.

Add conditionals to cover these scenarios and DRY up selectors within `withSelect`
@getdave getdave changed the title Nav Block - add Block placeholder Nav Block - improving initial state by adding Block placeholder Nov 11, 2019
@getdave getdave moved this from 💻 Issues in progress to 👀 PRs to review in Navigation block Nov 11, 2019
@mcsf
mcsf approved these changes Nov 11, 2019
@getdave getdave merged commit 19626b2 into master Nov 11, 2019
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 Nov 11, 2019
@getdave getdave deleted the try/nav-block-improve-initial-state branch Nov 11, 2019
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 11, 2019

Thanks to everyone who helped review this one. Appreciate your patience.

@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Nov 11, 2019

Thanks for all the improvements!

CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
…Press#18363)

* Adds initial placeholder with basic state toggle

* Updates to only display placeholder if there are no existing Nav Items

* Update to tidy and simplify component organisation and logic

The component was becoming difficult to navigation. Organise into sections with clear comments, colocate similar items and simplify some conditionals.

* Updates visual design and copy

Addresses WordPress#18307 (comment)

* Updates variable naming in anticipation of other data types

* Update don’t screen innerBlock unecessarily and simply bool prop.

Addresses

* WordPress#18363 (comment)
* WordPress#18363 (comment)

* Updates to simplify withSelect and idioms around this

See WordPress#18363 (comment)

* Simplifies JSX booleans

* Move rendered Block render attributes to be assigned only when used

Addresses WordPress#18363 (comment)

* Update design to match Gallery Block

* Updates implementation to remove state and utilise dispatch to update innerBlocks

After a long discussion with @mtias we realised that unlike other similar implementations (Media+Text and Columns) of a placeholder, this one couldn’t use the “Template Options” feature of <InnerBlocks> to achieve the custom design required. Instead it rendered a `<Placeholder />` directly based on conditions.

As a result, we now use the `replaceInnerBlocks` from `block-editor` to dispatch an update to innerBlocks manually when the placeholder buttons are clicked.

Addresses

* WordPress#18363 (comment)
* WordPress#18363 (comment)
* WordPress#18363 (comment)

* Fix to avoid improper use of `registry` when `select` is most appropriate.

Addresses WordPress#18363 (comment)

* Updates to move button styling directly to component.

Acting on advice from @mtias.

* Refactor to allow “Automatically Add” Inspector Control to remove Placeholder

* Fix bug where spinner was displayed even for empty menus

Addresses WordPress#18363 (comment)

* Fix to only allow creation from Pages if available.

* Update to improve safeguards around creation of Menus from Pages before Page data is available

Previously we didn’t know when the pages query had resolved. Moreover, we were allowing the Inspector controls to create Menus from Pages before the data was available.

Add conditionals to cover these scenarios and DRY up selectors within `withSelect`
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.