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.

Sign up for GitHub

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
+24 −25
Open

Stabilize the Experimental block supports property #65912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions packages/block-editor/src/hooks/border.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

gutenberg/packages/blocks/src/store/selectors.js

Lines 629 to 645 in 351e092

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 •
edited
Loading

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.

getBlockSupport( name, [ BORDER_SUPPORT_KEY, '__experimentalDefaultControls' ] ) ||
getBlockSupport( name, [ BORDER_SUPPORT_KEY, 'defaultControls' ] );

const defaultShadowControls =
getBlockSupport( name, [ SHADOW_SUPPORT_KEY, '__experimentalDefaultControls' ] ) ||
getBlockSupport( name, [ SHADOW_SUPPORT_KEY, 'defaultControls' ] );

const defaultControls = {
...getBlockSupport( name, [
BORDER_SUPPORT_KEY,
'__experimentalDefaultControls',
] ),
...getBlockSupport( name, [
SHADOW_SUPPORT_KEY,
'__experimentalDefaultControls',
] ),
...defaultBorderControls,
...defaultShadowControls,
};

return (
Expand Down
7 changes: 3 additions & 4 deletions packages/block-editor/src/hooks/color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,10 +288,9 @@ export function ColorEdit( { clientId, name, setAttributes, settings } ) {
return null;
}

const defaultControls = getBlockSupport( name, [
COLOR_SUPPORT_KEY,
'__experimentalDefaultControls',
] );
const defaultControls =
getBlockSupport( name, [ COLOR_SUPPORT_KEY, '__experimentalDefaultControls' ] ) ||
getBlockSupport( name, [ COLOR_SUPPORT_KEY, 'defaultControls' ] );

const enableContrastChecking =
Platform.OS === 'web' &&
Expand Down
17 changes: 8 additions & 9 deletions packages/block-editor/src/hooks/dimensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,19 +86,18 @@ export function DimensionsPanel( { clientId, name, setAttributes, settings } ) {
return null;
}

const defaultDimensionsControls = getBlockSupport( name, [
DIMENSIONS_SUPPORT_KEY,
'__experimentalDefaultControls',
] );
const defaultSpacingControls = getBlockSupport( name, [
SPACING_SUPPORT_KEY,
'__experimentalDefaultControls',
] );
const defaultDimensionsControls =
getBlockSupport( name, [ DIMENSIONS_SUPPORT_KEY, '__experimentalDefaultControls' ] ) ||
getBlockSupport( name, [ DIMENSIONS_SUPPORT_KEY, 'defaultControls' ] );

const defaultSpacingControls =
getBlockSupport( name, [ SPACING_SUPPORT_KEY, '__experimentalDefaultControls' ] ) ||
getBlockSupport( name, [ SPACING_SUPPORT_KEY, 'defaultControls' ] );

const defaultControls = {
...defaultDimensionsControls,
...defaultSpacingControls,
};

return (
<>
<StylesDimensionsPanel
Expand Down
7 changes: 3 additions & 4 deletions packages/block-editor/src/hooks/typography.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,9 @@ export function TypographyPanel( { clientId, name, setAttributes, settings } ) {
return null;
}

const defaultControls = getBlockSupport( name, [
TYPOGRAPHY_SUPPORT_KEY,
'__experimentalDefaultControls',
] );
const defaultControls =
getBlockSupport( name, [ TYPOGRAPHY_SUPPORT_KEY, '__experimentalDefaultControls' ] ) ||
getBlockSupport( name, [ TYPOGRAPHY_SUPPORT_KEY, 'defaultControls' ] );

return (
<StylesTypographyPanel
Expand Down
Loading