Extract the getSupportedStyles selector to the blocks store as a private selector #47606
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
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
noisysocks,
ramonjd,
jorgefilipecosta and
andrewserong
|
Size Change: +1.6 kB (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 81c1807. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4082324020
|
|
The failing unit test is a bit weird here, I think it's somehow related to "mocking" and "private selectors in store". For some reason the mocking is not working properly. Can you help @adamziel ? |
|
@youknowriad |
noisysocks
reviewed
Feb 1, 2023
There was a problem hiding this comment.
Nice! I like the hooks and that you added unit tests.
It seems to me that getSupportedStyles is two functions smooshed together:
(1) If name is provided then it looks up what styles are supported via getBlockType( name ).supports.
(2) If element is provided then it computes what styles are supported using ROOT_BLOCK_SUPPORTS and some custom logic.
Do you think it would be easier to reason about if we split them apart?
So have wp.data.select( 'core/blocks' ).getSupportedStyles( name ) which does (1) and then maybe wp.blockEditor.getSupportedStyles( element ) which does (2).
Then wp-blocks doesn't have to know what an "element" is which doesn't really feel right to me.
Lmkwyt. I might be misunderstanding something!
| } | ||
| } ); | ||
|
|
||
| return filterElementBlockSupports( supportKeys, name, element ); |
There was a problem hiding this comment.
Calling filterElementBlockSupports here should have no effect, right? Because we know that name is truthy and all but one of the branches in filterElementBlockSupports contain a ! name check meaning they won't execute. The one exception is the support === 'fontSize' && element === 'heading' branch but I think that might never execute here either because otherwise there wouldn't be a Font size control in Styles → Blocks → Heading → Typography.
|
It's not easy to split this PR like you suggested because there's some logic that needs to know both at the same time. Like links have this particular style only in root or something like that. I think as it stands, element only impacts "root" but for me, this is just a temporary thing. In other words, when we added this logic, we only thought about the root use case but It doesn't seem crazy for me that we'd add "supports" for elements as well and have some logic like also note that root block supports also make sense even without passing an "element". I'm hesitant here. |
youknowriad
force-pushed
the
update/extracted-get-supported-styles
branch
from
52d4e32
to
bd27573
Compare
|
Thanks @adamziel I've managed to remove the mocking. |
|
After thinking about it, it's about getting the supported styles for a couple (element, blockName) when both of these arguments are nullable. In other words, while getting supports for an element make sense on its own, getting the supports for a block also make sense, what this function is about is the combination of both. It's also a private selector so we can adapt. |
jorgefilipecosta
approved these changes
Feb 2, 2023
| 'letterSpacing', | ||
| ]; | ||
|
|
||
| function filterElementBlockSupports( blockSuppots, name, element ) { |
There was a problem hiding this comment.
Could we include some minor docs for this function?
|
|
||
| function filterElementBlockSupports( blockSuppots, name, element ) { | ||
| return blockSuppots.filter( ( support ) => { | ||
| if ( support === 'fontSize' && element === 'heading' ) { |
There was a problem hiding this comment.
It would be nice to somehow encode these rules in a declarative approach as a JavaScript object. But unfortunately, that is not simple to do.
| STYLE_PROPERTY[ styleName ].support | ||
| ) !== false | ||
| ) { | ||
| return supportKeys.push( styleName ); |
There was a problem hiding this comment.
nitpick: given that a forEach does not returns, I would do:
supportKeys.push( styleName );
return;
To make it clear we are in a function that does not return.
| blockType.supports && | ||
| get( | ||
| blockType.supports, | ||
| STYLE_PROPERTY[ styleName ].support |
There was a problem hiding this comment.
Do we need this logic STYLE_PROPERTY[ styleName ].support[ 0 ] in blockType.supports ? Could we simply get(blockType.supports, STYLE_PROPERTY[ styleName ].support ) !== false without that part?
There was a problem hiding this comment.
To be honest I'm not entirely sure, this was just copied as is.
DaisyOlsen
added
[Package] Editor
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.
Extracted from #47356
What?
This is a small refactoring that extracts the
getSupportedStylesfunction as a private selector into the blocks store.Why?
In #47356, we moved the
getSupportedGlobalStylesPanelsfunction to the block-editor package but in reality it's better suited for the blocks package because it only needs the block registration.Also, I'd like to use this function in a follow-up to build a "theme.json" like settings object based on a blockName, selector pair using this function as a starting point.
See related discussion #47356 (comment) and #47356 (comment)
Testing Instructions
1- Open the global styles UI (especially typography)
2- Ensure that they show the exact same panels as in trunk.
I've also added some unit tests to this function.