Allow setting a `defaultBlock` attribute on the navigation block #51026

sethrubenstein · 2023-05-27T03:04:41Z

What?

This adds a new defaultBlock attribute to allow site owners to customize the core/navigation block experience for end users.

Why?

This solves for #50982

How?

This adds a new defaultBlock attribute and a check for it along with a quick sanity check if it fails it falls back to the DEFAULT_BLOCK constant.

Testing Instructions

Open a post or page
Copy and paste this block markup into the editor:

<!-- wp:navigation {"defaultBlock":{"name":"core/navigation-link","attributes":{"type":"category","kind":"taxonomy"}}} /-->

Click + inside the navigation block.
Now category links should be the default when clicking + inside this navigation block.

Testing Instructions for Keyboard

n/a

github-actions · 2023-05-27T03:05:03Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @sethrubenstein! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

paaljoachim · 2023-06-16T15:47:38Z

Thank you very much for contributing @sethrubenstein Seth! It is much appreciated!

I will go ahead and ping @getdave and @MaggieCabrera so they know about this PR and can check it out.

sethrubenstein · 2023-06-16T15:58:31Z

@paaljoachim Thanks! I look forward to any feedback!

talldan

Thanks for your contribution @sethrubenstein.

I left a couple of comments. The biggest issue seems to be that __experimentalDefaultBlock isn't a stable API, so I don't think we can comfortably expose it via the nav block's attributes.

I think there's some work to do to stabilize __experimentalDefaultBlock first.

talldan · 2023-06-19T06:02:11Z

packages/block-library/src/navigation/edit/inner-blocks.js

+	// If the `defaultBlock` attribute is set and itself has an attributes object with values then use it
+	// otherwise fallback to the DEFAULT_BLOCK constant.
+	// This allows site owners to set the default `core/navigation-link` variation to use.
+	const navDefaultBlock = defaultBlock && defaultBlock.attributes && Object.keys(defaultBlock.attributes).length > 0 ? defaultBlock : DEFAULT_BLOCK;


I'm not sure about the part that checks the length of the attributes. Wouldn't that disallow setting a new default that doesn't have any specific default attributes?

@talldan Thats correct, my intention here was to assume if a developer/user hasn't included some attributes that actually changes something about the default core/navigation-link block to be inserted that we'd default to DEFAULT_BLOCK. But I can also see how that might confuse people at first. Ideally I think we'd get rid of DEFAULT_BLOCK altogether and rely on the block.json defaultBlock value... but I didn't want to assume I could unilaterally make that call.

So I'm completely open to however you all want this to work this was just my very opinionated approach to respecting the default behavior already present.

talldan · 2023-06-19T06:03:06Z

packages/block-library/src/navigation/edit/inner-blocks.js

@@ -98,7 +104,7 @@ export default function NavigationInnerBlocks( {
 			onChange,
 			allowedBlocks: ALLOWED_BLOCKS,
 			prioritizedInserterBlocks: PRIORITIZED_INSERTER_BLOCKS,
-			__experimentalDefaultBlock: DEFAULT_BLOCK,
+			__experimentalDefaultBlock: navDefaultBlock,


Given this was introduced as an experimental API, I'm not sure about exposing it as a stable attribute of a block.

It might be worth stabilizing the __experimentalDefaultBlock API first so that this becomes defaultBlock: navDefaultBlock and deprecating the __experimentalDefaultBlock property.

Resolved by #52083

talldan · 2023-06-19T06:08:49Z

packages/block-library/src/navigation/edit/inner-blocks.js

@@ -98,7 +104,7 @@ export default function NavigationInnerBlocks( {
 			onChange,
 			allowedBlocks: ALLOWED_BLOCKS,
 			prioritizedInserterBlocks: PRIORITIZED_INSERTER_BLOCKS,
-			__experimentalDefaultBlock: DEFAULT_BLOCK,
+			__experimentalDefaultBlock: navDefaultBlock,
 			__experimentalDirectInsert: shouldDirectInsert,


In terms of stabilizing __experimentalDefaultBlock, I think it could be worth looking at stabilizing __experimentalDirectInsert too, as I believe the two props were designed to work together.

Perhaps if there's consensus that defaultBlock should be added as a block attribute, the same should also apply to directInsert. 🤔

Resolved by #52083

getdave · 2023-06-19T08:11:01Z

Thanks for your contribution @sethrubenstein.

I left a couple of comments. The biggest issue seems to be that __experimentalDefaultBlock isn't a stable API, so I don't think we can comfortably expose it via the nav block's attributes.

I think there's some work to do to stabilize __experimentalDefaultBlock first.

@talldan I think we're at a point where those two "__experimental" properties have been used extensively for several releases. Based on that do you agree we can stablise them for 6.3?

If so next steps would be:

PR to stablise the two APIs
Rebase this PR

talldan · 2023-06-19T08:15:55Z

Looks ok to stabilize to me 👍

Not sure if there's anyone else you want to run it past. I think @tellthemachines originally implemented it, but hasn't worked on these blocks for a while.

getdave · 2023-06-20T17:16:31Z

I'll just say now that - as much as I'd like to - I will not have capacity to work on this in time for WordPress 6.3. Happy to work on it afterwards though.

sethrubenstein · 2023-06-20T19:10:51Z

@getdave I'm not sure if you want to give first time contributors such a large ticket but I'd be happy to offer up some of our teams time to stabilize __experimentalDefaultBlock and __experimentalDirectInsert if that would be of help or desirable? Complete transparency, not sure what that work would entail beyond getting rid of the __experimental wording across Gutenberg for these two, setting the default values in block.json where in use (looks like buttons and navigation right now), and of course testing; but we have the labor and are available.

getdave · 2023-06-21T09:30:10Z

@sethrubenstein That would be fantastic. Thank you.

I would recommend separate PRs for:

__experimentalDefaultBlock
__experimentalDirectInsert

My understanding is that we would remove the prefix, but also retain the original in its entirity for backwards compatibility purposes. We would likely also add deprecated calls to the __experimental function to encourage folks to migrate.

This is a procedure for legacy code, whereas in future all "experiments" will be hidden via the private apis mechanism that is now available.

talldan · 2023-06-21T10:20:04Z

This PR might serve as an example the process - #47183. Though it's stabilizing a slightly different API, it still showcases keeping the original with the __experimental prefix but adding a deprecation message to it.

sethrubenstein · 2023-06-21T12:23:24Z

@getdave @talldan Sounds good, we'll try and get PR's for that work together in the next week.

sethrubenstein · 2023-06-28T16:54:56Z

@getdave @talldan I have a PR ready for stabilizing those API's and related selectors: #52083.

I'll re-work this pr to get rid of the __experimental references, and I think get rid of the DEFAULT_BLOCK constant in favor of the block.json defined defaultBlock for core/navigation once that PR gets merged into trunk probably best?

packages/block-library/src/navigation/edit/inner-blocks.js

* Add new state handlers for setting Nav Menu Fixing unlocking Avoid unnecessarily effectful code Fix bug with persistent Nav block locking Reinstate effect to limit calls to selectBlock Reinstate effect to run selectively Isolate Nav specific code Refactor settings to hook Isolate Editor Canvas to component Extract mode statuses to hook Colocate editor canvas dependencies Remove requirement for Nav hook to return data Extract entire canvas to component Get blocks directly from the store Use factory to provide default editor component Extract independent components for default and wp_navigation Remove template dep from Navigation focus hook Delete redundant hook Remove need for settings prop Extract hooks to files Export boolean to avoid render Remove redundant prop and var Extract Site Editor Canvas component to file Extract factory to file Remove need to pass props to abstract component Improve abstract component and factory naming Improve usage of constants Be explicit about entity mapping in factory Remove templateType prop from SiteEditorCanvas component Improve variable naming Use more performant selector Improve documenting rule for showing appender Move Navigation specific editor settings to relevant provider Remove useSiteEditorMode hook See WordPress#39286 (comment) Reintstate bug fix with comment Fix error in rebase Add edit button Use Navigation icon and label Use correct labels * Add descriptive text as instructed See WordPress#39286 * Update edit link for Navigation post type Fixes revisions link * Fix PHPCS

…ss#51562)

…ss#51254) * Update: Adjust modal radius to be between frame and buttons. * Update changelog. * Try moving changelog.

…51571)

…rdPress#51463) * Mobile - Image block - Fix issue with set width and height images * Mobile - Update changelog

sethrubenstein · 2023-07-13T16:19:03Z

Oy vey, I messed up my merge. Going to open a new pull request with this change reflecting the latest from trunk. I'll open a new PR shortly @getdave

sethrubenstein · 2023-07-13T16:27:34Z

Sorry for the mess, new PR here @getdave #52610

sethrubenstein requested a review from tellthemachines as a code owner May 27, 2023 03:04

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label May 27, 2023

Mamaduka added this to 👀 PRs needing review in Navigation block via automation May 30, 2023

skorasaurus added the [Block] Navigation Affects the Navigation Block label Jun 1, 2023

mtias added the [Feature] Extensibility The ability to extend blocks or the editing experience label Jun 2, 2023

ndiego linked an issue Jun 5, 2023 that may be closed by this pull request

Allow selecting the default core/navigation-link variant in the core/navigation block #50982

Closed

Mamaduka requested review from scruffian and getdave June 16, 2023 16:08

ndiego added the [Type] Enhancement A suggestion for improvement. label Jun 16, 2023

talldan reviewed Jun 19, 2023

View reviewed changes

This was referenced Jun 28, 2023

Stabilize __experimentalDefaultBlock and __experimentalDirectInsert APIs #52068

Closed

Stabilize defaultBlock, directInsert API's and getDirectInsertBlock selector #52083

Merged

getdave reviewed Jul 13, 2023

View reviewed changes

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

getdave and others added 5 commits July 13, 2023 12:15

Code Data: Fix ESLint warning for the 'useEntityRecord' hook (WordPre…

9f4ea09

…ss#51562)

Update: Adjust modal radius to be between frame and buttons. (WordPre…

dba2afe

…ss#51254) * Update: Adjust modal radius to be between frame and buttons. * Update changelog. * Try moving changelog.

Remove the hydration console log of the Interactivity API (WordPress#…

d001b55

…51571)

Mobile - Image block - Fix issue with set width and height images (Wo…

eee7e92

…rdPress#51463) * Mobile - Image block - Fix issue with set width and height images * Mobile - Update changelog

sethrubenstein closed this Jul 13, 2023

Navigation block automation moved this from 👀 PRs needing review to ✅ Done Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting a `defaultBlock` attribute on the navigation block #51026

Allow setting a `defaultBlock` attribute on the navigation block #51026

sethrubenstein commented May 27, 2023

github-actions bot commented May 27, 2023

paaljoachim commented Jun 16, 2023 •

edited

Loading

sethrubenstein commented Jun 16, 2023

talldan left a comment

talldan Jun 19, 2023

sethrubenstein Jun 20, 2023

talldan Jun 19, 2023

getdave Jul 13, 2023

talldan Jun 19, 2023

getdave Jul 13, 2023

getdave commented Jun 19, 2023

talldan commented Jun 19, 2023

getdave commented Jun 20, 2023

sethrubenstein commented Jun 20, 2023

getdave commented Jun 21, 2023

talldan commented Jun 21, 2023

sethrubenstein commented Jun 21, 2023

sethrubenstein commented Jun 28, 2023

sethrubenstein commented Jul 13, 2023

sethrubenstein commented Jul 13, 2023

Allow setting a defaultBlock attribute on the navigation block #51026

Allow setting a defaultBlock attribute on the navigation block #51026

Conversation

sethrubenstein commented May 27, 2023

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

github-actions bot commented May 27, 2023

paaljoachim commented Jun 16, 2023 • edited Loading

sethrubenstein commented Jun 16, 2023

talldan left a comment

Choose a reason for hiding this comment

talldan Jun 19, 2023

Choose a reason for hiding this comment

sethrubenstein Jun 20, 2023

Choose a reason for hiding this comment

talldan Jun 19, 2023

Choose a reason for hiding this comment

getdave Jul 13, 2023

Choose a reason for hiding this comment

talldan Jun 19, 2023

Choose a reason for hiding this comment

getdave Jul 13, 2023

Choose a reason for hiding this comment

getdave commented Jun 19, 2023

talldan commented Jun 19, 2023

getdave commented Jun 20, 2023

sethrubenstein commented Jun 20, 2023

getdave commented Jun 21, 2023

talldan commented Jun 21, 2023

sethrubenstein commented Jun 21, 2023

sethrubenstein commented Jun 28, 2023

sethrubenstein commented Jul 13, 2023

sethrubenstein commented Jul 13, 2023

Allow setting a `defaultBlock` attribute on the navigation block #51026

Allow setting a `defaultBlock` attribute on the navigation block #51026

paaljoachim commented Jun 16, 2023 •

edited

Loading