Nav Block - improving initial state by adding Block placeholder and ability to create from existing Pages. #18363

getdave · 2019-11-07T11:01:50Z

Closes #18307.

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

Create a Menu from existing pages.
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

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.

packages/block-library/src/navigation-menu/edit.js

mcsf

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:

packages/block-library/src/navigation-menu/edit.js

karmatosed · 2019-11-07T21:32:17Z

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

packages/block-library/src/navigation-menu/edit.js

Addresses * #18363 (comment) * #18363 (comment)

See #18363 (comment)

Addresses #18363 (comment)

getdave · 2019-11-08T11:28:09Z

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:

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

getdave · 2019-11-08T11:30:36Z

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

packages/block-library/src/navigation-menu/editor.scss

@mtias

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

…iate. Addresses #18363 (comment)

getdave · 2019-11-08T14:39:39Z

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.

See #18363 (comment)

Addresses #18363 (comment)

@mtias

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

…iate. Addresses #18363 (comment)

@mtias

Acting on advice from @mtias.

…ceholder

karmatosed · 2019-11-08T23:02:20Z

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

No that looks fine to me now, thanks.

karmatosed

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

mtias · 2019-11-10T16:55:57Z

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

Addresses #18363 (comment)

getdave · 2019-11-11T10:41:44Z

@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

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

I've now fixed this issue.

getdave · 2019-11-11T10:44:46Z

New Bug:

Load a new blank Post
Throttle network speed in devtools to Slow 3G.
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.

…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 · 2019-11-11T12:09:41Z

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

mtias · 2019-11-11T18:46:00Z

Thanks for all the improvements!

getdave self-assigned this Nov 7, 2019

getdave added this to 👀 PRs to review in Navigation editor via automation Nov 7, 2019

getdave marked this pull request as ready for review November 7, 2019 14:03

getdave requested review from ajitbohra, jorgefilipecosta, Soean and talldan as code owners November 7, 2019 14:03

getdave requested review from draganescu, mtias and karmatosed November 7, 2019 14:04

getdave added [Block] Navigation Affects the Navigation Block Needs Design Feedback Needs general design feedback. Needs Technical Feedback Needs testing from a developer perspective. labels Nov 7, 2019

getdave mentioned this pull request Nov 7, 2019

Navigation Block: Improve initial state. #18307

Closed

2 tasks

mtias reviewed Nov 7, 2019

View reviewed changes

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

getdave changed the title ~~Adds initial placeholder with basic state toggle~~ Nav Block - add Block placeholder Nov 7, 2019

gziolo mentioned this pull request Nov 7, 2019

Variations (formerly patterns) API for blocks #16283

Closed

16 tasks

mcsf requested changes Nov 7, 2019

View reviewed changes

talldan reviewed Nov 8, 2019

View reviewed changes

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

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

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

9acee9b

Addresses * #18363 (comment) * #18363 (comment)

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

Updates to simplify withSelect and idioms around this

abfb839

See #18363 (comment)

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

Move rendered Block render attributes to be assigned only when used

aa00a8a

Addresses #18363 (comment)

mtias reviewed Nov 8, 2019

View reviewed changes

packages/block-library/src/navigation-menu/editor.scss Outdated Show resolved Hide resolved

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

Fix to avoid improper use of registry when select is most appropr…

183df4d

…iate. Addresses #18363 (comment)

getdave requested a review from mcsf November 8, 2019 14:44

getdave added 8 commits November 8, 2019 15:59

Updates to simplify withSelect and idioms around this

b463219

See #18363 (comment)

Simplifies JSX booleans

dd5c89a

Move rendered Block render attributes to be assigned only when used

ccb4691

Addresses #18363 (comment)

Update design to match Gallery Block

b742704

Fix to avoid improper use of registry when select is most appropr…

33b6417

…iate. Addresses #18363 (comment)

Updates to move button styling directly to component.

9a04b16

Acting on advice from @mtias.

Refactor to allow “Automatically Add” Inspector Control to remove Pla…

b287122

…ceholder

getdave force-pushed the try/nav-block-improve-initial-state branch from 9c2b488 to b287122 Compare November 8, 2019 16:00

getdave requested review from mcsf and mtias November 8, 2019 16:01

karmatosed approved these changes Nov 8, 2019

View reviewed changes

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

Fix bug where spinner was displayed even for empty menus

30266b0

Addresses #18363 (comment)

getdave added 2 commits November 11, 2019 10:51

Fix to only allow creation from Pages if available.

9adaf0f

getdave changed the title ~~Nav Block - add Block placeholder~~ Nav Block - improving initial state by adding Block placeholder Nov 11, 2019

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

mcsf approved these changes Nov 11, 2019

View reviewed changes

getdave merged commit 19626b2 into master Nov 11, 2019

Navigation editor automation moved this from 👀 PRs to review to ✅ Done Nov 11, 2019

getdave deleted the try/nav-block-improve-initial-state branch November 11, 2019 12:09

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

getdave changed the title ~~Nav Block - improving initial state by adding Block placeholder~~ Nav Block - improving initial state by adding Block placeholder and ability to create from existing Pages. Apr 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nav Block - improving initial state by adding Block placeholder and ability to create from existing Pages. #18363

Nav Block - improving initial state by adding Block placeholder and ability to create from existing Pages. #18363

getdave commented Nov 7, 2019 •

edited

Loading

mcsf left a comment

karmatosed commented Nov 7, 2019

getdave commented Nov 8, 2019

getdave commented Nov 8, 2019

getdave commented Nov 8, 2019

karmatosed commented Nov 8, 2019

karmatosed left a comment

mtias commented Nov 10, 2019

getdave commented Nov 11, 2019

getdave commented Nov 11, 2019 •

edited

Loading

getdave commented Nov 11, 2019

mtias commented Nov 11, 2019

Nav Block - improving initial state by adding Block placeholder and ability to create from existing Pages. #18363

Nav Block - improving initial state by adding Block placeholder and ability to create from existing Pages. #18363

Conversation

getdave commented Nov 7, 2019 • edited Loading

Questions

How has this been tested?

Screenshots

Types of changes

Checklist:

mcsf left a comment

Choose a reason for hiding this comment

karmatosed commented Nov 7, 2019

getdave commented Nov 8, 2019

getdave commented Nov 8, 2019

getdave commented Nov 8, 2019

karmatosed commented Nov 8, 2019

karmatosed left a comment

Choose a reason for hiding this comment

mtias commented Nov 10, 2019

getdave commented Nov 11, 2019

getdave commented Nov 11, 2019 • edited Loading

Update

getdave commented Nov 11, 2019

mtias commented Nov 11, 2019

getdave commented Nov 7, 2019 •

edited

Loading

getdave commented Nov 11, 2019 •

edited

Loading