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

Stabilize the Experimental block supports property #65912

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

alamgir-multidots
Copy link

@alamgir-multidots alamgir-multidots commented Oct 7, 2024

What?

Split from #64314

Why?

The __experimentalDefaultControls property developers can choose which settings are displayed by default in the Editor for each block support. The __experimental prefix removed and now make it easier for third-party developers to confidently use this defaultControls property. Also, falling back to the __experimental prefix if available.

__experimentalDefaultControls → defaultControls

I have added the supports for those hooks:

  • Color
  • Typography
  • Spacing
  • Border

Now both are workable __experimentalDefaultControls & defaultControls

Testing Instructions

  1. Open block.json file or area.
  2. If you have already used for block support __experimentalDefaultControls instead of we can use it defaultControls only.
  3. Block support default controls now managing by using this defaultControls property

Note: Here we need to update the WP core blocks' block.json files to use the non __experimental prefixes.

Copy link

github-actions bot commented Oct 7, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @“alamgir.hossain@multidots.com”.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: “alamgir.hossain@multidots.com”.

Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: ndiego <ndiego@git.wordpress.org>
Co-authored-by: akshaya-rane <akshayar@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

github-actions bot commented Oct 7, 2024

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @alamgir-multidots! In case you missed it, we'd love to have you join us in our Slack community.

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

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Oct 7, 2024
@ndiego ndiego added [Package] Block library /packages/block-library [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Oct 10, 2024
@ndiego
Copy link
Member

ndiego commented Oct 10, 2024

Thank you for working on this @alamgir-multidots!

I just tested it with my own Icon Block, which is currently using __experimentalDefaultControls in a few places. Below is the specification for border, all controls should be disabled by default.

"__experimentalBorder": {
      "color": true,
      "radius": true,
      "style": true,
      "width": true,
      "__experimentalSelector": ".icon-container",
      "__experimentalSkipSerialization": true,
      "__experimentalDefaultControls": {
            "color": false,
            "radius": false,
            "style": false,
            "width": false
      }
} 

As you can see in the screenshots below, the controls remain hidden with this PR when __experimentalDefaultControls is set and when defaultControls is set. 🎉

With __experimentalDefaultControls With defaultControls
image image

@Mamaduka or @gziolo would you be able to do a more comprehensive code review?

I do want to note that perhaps as a follow-up to this PR, we will need to update all Core blocks that currently use __experimentalDefaultControls. I'm also wondering if a deprecation warning in the console should also be added 🤔

@ndiego ndiego added the [Type] Enhancement A suggestion for improvement. label Oct 10, 2024
@ndiego
Copy link
Member

ndiego commented Oct 24, 2024

While waiting for a code review, @alamgir-multidots, were you planning on creating follow-up PRs to address the additional tasks in the original issue? I imagine that's probably easier than bundling everything into one PR.

  • Update the block.json schema to support the non __experimental prefix
  • Update the core blocks' block.json files to use the non __experimental prefixes
  • Update the supports documentation to include a section about default controls.
  • Update the block.json schema to remove the __experimental prefix. Note that the code in WordPress should still support the __experimental prefix, but at this stage, the experimental prefix will be treated as deprecated.

@@ -160,15 +160,17 @@ export function BorderPanel( { clientId, name, setAttributes, settings } ) {
return null;
}

const defaultBorderControls =
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, have you considered updating the core/blocks store to be able to fall back to the experimental name when the new name is not found? In effect, in all places for the edited files, we would only need to use the non-experimental name. In addition to that, getBlockSupport would handle the deprecated name as needed:

export const getBlockSupport = (
state,
nameOrType,
feature,
defaultSupports
) => {
const blockType = getNormalizedBlockType( state, nameOrType );
if ( ! blockType?.supports ) {
return defaultSupports;
}
return getValueFromObjectPath(
blockType.supports,
feature,
defaultSupports
);
};

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this idea. Ideally it'd be good if we can handle the fallback to the experimental name within getBlockSupport (or the store) rather than each of the block supports needing to know about the two different property names.

That said, this PR also isn't a lot of code, so if it winds up being complex to implement I don't mind the duplication too much — but it would be good to neaten up if it's feasible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is a great hurry to stabilize these so I'd vote for doing it once properly for all block supports.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @gziolo.,

I am continuing with this issue and feedback from you in a new PR I have updated the getBlockSupport function as mentioned by you. Please have a look and let you know your feedback.

cc @andrewserong, @ndiego

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw Nov 18, 2024

Choose a reason for hiding this comment

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

Hi @karthick-murugan 👋

Theres been some unfortunate overlap of efforts to stabilizing block supports lately. The general consensus has been to consolidate stabilization of block supports around the approach pioneered in #63401.

I already have a PR close to landing for the border block supports in #66918. The hold up in landing that was I was also iterating there so that the same stabilizing functions and filters also took care of both __experimentalDefaultControls and __experimentalSkipSerialization.

For that I already have a draft available, #67018.

At this stage, I should have the finalized PRs up today or tomorrow for review. Rather than scatter the stabilization of block supports across various utils and selectors. I strongly believe we should have a consistent location for handling them.

I'll drop a comment over on #67073 as well with the hope to get us all on the same page again.

P.S. My apologies for not including __experimentalDefaultControls etc in the title of my PR, if that lead to the duplicated PR effort. My thinking was that Adding both the experimental flags to the title would be too long.

@gziolo
Copy link
Member

gziolo commented Oct 25, 2024

I was cross-checking at #63401, where @andrewserong, with the help of @aaronrobertshaw, took a slightly different approach to stabilizing typography block supports. I'm not entirely sure if my comment #65912 (comment) makes perfect sense then, so let's double check with them first.

@andrewserong
Copy link
Contributor

andrewserong commented Oct 28, 2024

Thanks for the ping @gziolo (and for working on this @alamgir-multidots)! From my perspective I think it's fine if the defaultControls property takes a slightly different approach to stabilization than the supports themselves (e.g. Typography in #63401 and Border in #65968) as default controls are only used in JS, so I think it can be handled in a slightly simpler way.

Overall I like the idea of your comment in #65912 (comment) — the way I imagine it'd work is that the overall block support itself would eventually get stabilized as in #63401, and then when that block support is interacted with via getBlockSupport the fallback handling of defaultControls and __experimentalDefaultControls would happen.

Does that make sense / sound viable to you?

The only other concern I can think of is whether we need to support plugins that may or may not be filtering or augmenting the value for __experimentalDefaultControls. My hunch is that it's probably a lesser concern than filtering for the block supports themselves (what we were looking at with the Typography support), as this is about the default display of controls rather than whether or not they're present at all.

@aaronrobertshaw
Copy link
Contributor

Thanks for the ping @gziolo (and for working on this @alamgir-multidots)!

+1

The only other concern I can think of is whether we need to support plugins that may or may not be filtering or augmenting the value for __experimentalDefaultControls.

This would be my concern as well.

One benefit in addressing this is that it would make this consistent with the stabilization of other block support properties.

My hunch is that when others come to stabilizing more block supports, the "simplest" block support example found will be used. Ensuring completeness for the stabilization here would help guide and inform those future efforts too.

It's also still not clear to me when plugins filter all this, which keys should be taking priority. For example, if we migrate a core block to use the stabilized key but a plugin filtering that still uses the experimental key. I'd expect the plugin's filter to still take effect for backwards compability.

What about a scenario then where two plugins filtered the same property? The first to filter it uses the experimental form, then the second uses the stabilized key, or vice-versa.

That might be a contrived example but worth considering.

@akshaya-rane
Copy link

Hi @ndiego,

Thank you for your patience.

We will take care of it & update here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Block library /packages/block-library [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants