Stabilise role attribute property #65484
Merged
+250
−182
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
getdave
added
[Type] Code Quality
getdave
commented
Sep 19, 2024
Comment on lines
-127
to
+130
| const __experimentalHasContentRoleAttribute = jest.fn( () => false ); | ||
| const hasContentRoleAttribute = jest.fn( () => false ); | ||
| getBlockEditingMode.registry = { | ||
| select: jest.fn( () => ( { | ||
| __experimentalHasContentRoleAttribute, | ||
| hasContentRoleAttribute, |
There was a problem hiding this comment.
Should these tests be moved to selectors tests now?
getdave
commented
Sep 19, 2024
|
Size Change: +520 B (+0.03%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Sep 19, 2024
getdave
requested review from
ellatrix,
artemiomorales,
ajitbohra and
nerrad
as code owners
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
getdave
commented
Sep 20, 2024
gziolo
reviewed
Sep 20, 2024
youknowriad
added
the
Needs Dev Note
getdave
commented
Sep 20, 2024
|
Looking at the unit tests now |
I think that's a decision that's up for debate. The "Zoom Out mode" feature does make use of contentOnly but not in a way that requires us to know about which blocks are "content blocks". The recent "Select mode" work uses We might want to buy some more time for the API to stablise alongside the new modes, or alternatively we could say the API that's been around for 2yrs is probably good to stablise. Opinions welcomed. |
Anyone have any idea why this test is failing on CI but passing locally? Going to try a rebase. |
Unit tests seem a legitimate failure. E2e tests might be a regression discussed on WP Slack (link requires registration at https://make.wordpress.org/chat/): |
I agree. The problem is with the following test: You can run this locally with the following" Problem
const { hasContentRoleAttribute } = unlock(
select( blocksStore )
);
const isContent = hasContentRoleAttribute( name );
const hasContentRoleAttribute = jest.fn(() => false);
getBlockEditingMode.registry = {
select: jest.fn(() => ({
hasContentRoleAttribute,
})),
};This is slightly confusing but essentially it's providing the gutenberg/packages/data/src/factory.js Line 53 in efd622d
What have I tried so far?See 37a0014 |
|
@draganescu @gziolo I think I may have found a solution to the issue outlined above. Essentially I've:
This means that when the actual implementation calls: const { hasContentRoleAttribute } = unlock(
select( blocksStore )
);...the result of I would appreciate a confidence check on that though 😅 As part of this I found one of the tests was failing. I looked into it and discovered that the test would always fail because the mock would always return |
getdave
commented
Sep 24, 2024
Comment on lines
4645
to
+4654
| it( 'in navigation mode, blocks with content attributes within sections are contentOnly', () => { | ||
| hasContentRoleAttribute.mockReturnValueOnce( true ); | ||
| expect( | ||
| getBlockEditingMode( | ||
| navigationModeStateWithRootSection, | ||
| 'b3247f75-fd94-4fef-97f9-5bfd162cc416' | ||
| ) | ||
| ).toBe( 'contentOnly' ); | ||
|
|
||
| hasContentRoleAttribute.mockReturnValueOnce( true ); |
There was a problem hiding this comment.
I think this test needs to include a test case that asserts the inverse cases. What do you think?
draganescu
approved these changes
Sep 24, 2024
|
|
||
| const fauxPrivateAPIs = {}; | ||
|
|
||
| lock( fauxPrivateAPIs, { hasContentRoleAttribute } ); |
There was a problem hiding this comment.
This seems like a correct mock to me.
gziolo
approved these changes
Sep 24, 2024
There was a problem hiding this comment.
So nice to see it ready 👍🏻
The next step is going to be an update to the documentation that explains what the role property offers so custom blocks can start using it. The related document would be:
https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-attributes.md
getdave
added
the
Backport to WP 6.7 Beta/RC
|
There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch. PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk. |
|
I am not sure if the Why?
|
|
We've discussed this above a bit. For me it does make sense.
|
|
Now that you mentioned it, I checked the resolved discussions and saw it 😓
Of course. I'll create a follow-up PR to remove the alternative suggestion. |
This was referenced Sep 24, 2024
|
I just noticed this PR didn't cherry pick cleanly. I will do this manually as soon as I can. |
|
How about updating the block.json schema? Developers will not know what this stabilized field means. |
kevin940726
pushed a commit
that referenced
this pull request
Sep 27, 2024
* Update usages * Update related functions * Remove unwanted alias * Properly deprecate the experimental method * Deprecate and ensure backwards compatibility * Make selector private * Ensure serializer works backwards compat * Ensure experiment API method still exposed on blocks package * Add hint for updating role attribute * Remove unlock * Improve readability of proxying * Add hint to serializer * use stabilised selector for the content role for edit mode * Attempt locking to fix unit test * Ensure mock returns correct value --------- Co-authored-by: getdave <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: ndiego <[email protected]> Co-authored-by: fabiankaegy <[email protected]>
noisysocks
pushed a commit
that referenced
this pull request
Sep 30, 2024
) * Update usages * Update related functions * Remove unwanted alias * Properly deprecate the experimental method * Deprecate and ensure backwards compatibility * Make selector private * Ensure serializer works backwards compat * Ensure experiment API method still exposed on blocks package * Add hint for updating role attribute * Remove unlock * Improve readability of proxying * Add hint to serializer * use stabilised selector for the content role for edit mode * Attempt locking to fix unit test * Ensure mock returns correct value --------- Co-authored-by: Dave Smith <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: ndiego <[email protected]> Co-authored-by: fabiankaegy <[email protected]>
noisysocks
added
Backported to WP Core
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Stablises the
_experimentalRoleproperty of block attributes andallsome related selectors and utils.Note:
__experimentalHasContentRoleAttributehas been made a private selector. Full backwards compatibility will remain until WP 6.8.Why?
This has been in use for 2 years and has not be stablised. With all the work on
contentOnlygoing on it makes sense to look to stablise the APIs in order that block authors can be confident in making use of them.How?
__experimentalHasContentRoleAttributea private selector.__experimentalprefixed versions to new versions and throwsdepreciated()warning__experimentalRoleprop.__experimentalprefixed functions and attribute props.Testing Instructions
hasContentRoleAttributeAPI__experimentalGetBlockAttributesNamesByRoleAPI:The call to the experimental method should throw a deprecated notice.
Also check Editor doesn't crash when moving in / out of Zoom Out mode or the new "Edit" mode.
Registering a Block
Content Only Test Blockto the post and type something into the inputTesting Instructions for Keyboard
Screenshots or screencast