[Documentation] Add @examples to core/blocks selectors. #42572
Merged
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
getBlockSupport() hasBlockSupport() hasChildBlocks() hasChildBlocksWithInserterSupport
ryanwelcher
added
[Type] Developer Documentation
ryanwelcher
requested review from
ajitbohra,
juanmaguitar and
fabiankaegy
as code owners
19 tasks
ryanwelcher
changed the title
|
Size Change: -190 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
gziolo
reviewed
Jul 21, 2022
| * | ||
| * // Return the active block variation for the first block. | ||
| * return select( blocksStore ).getActiveBlockVariation( | ||
| * 'core/embed', |
There was a problem hiding this comment.
How about also using firstBlock for consistency?:
Suggested change
| * 'core/embed', | |
| * firstBlock.name, |
I think that it should also contain block attributes: firstBlock.attributes.
There was a problem hiding this comment.
I've simplified the whole example based on your suggestion - thanks!
gziolo
reviewed
Jul 21, 2022
| * | ||
| * const ExampleComponent = () => { | ||
| * const blockNameIsFound = useSelect( ( select ) => | ||
| * select( blocksStore ).isMatchingSearchTerm( 'core/navigation' ) |
There was a problem hiding this comment.
I think that the second param is missing here. You have to pass the search term to validate it against a given block type.
There was a problem hiding this comment.
Updated to provide a much clearer example and explain a little better where the search is happening.
| * | ||
| * const ExampleComponent = () => { | ||
| * const navigationBlockHasChildBlocksWithInserterSupport = useSelect( ( select ) => | ||
| * select( blocksStore ).hasChildBlocksWithInserterSupport( |
There was a problem hiding this comment.
I'm not sure how popular this function is, but it would be good to explore what role it plays and maybe hide it from the documentation to avoid the noise.
There was a problem hiding this comment.
I really have no idea where it's used. An equivalent function is exported here. Perhaps we should @ignore this one here? That might also be a good idea for all the data store selectors with wrappers in the api dir?
There was a problem hiding this comment.
hasChildBlocksWithInserterSupport was introduced 4 years ago, so it's hard to tell 😄Although, I see it is no longer used so we might want to mark it as deprecated in both places.
That might also be a good idea for all the data store selectors with wrappers in the api dir?
I don't think it's a good idea overall. All public methods are re-exported in https://github.com/WordPress/gutenberg/blob/trunk/packages/blocks/src/api/index.js. While it's convenient to run some of those selectors through the wrapper exported directly from @wordpress/blocks, in many cases, it would be anti-pattern when used in React components. The reason for that is that you lose all the handling for re-renders coming from useSelect when the return value changes over time.
|
@gziolo while making the update based on your notes, I discovered that the docs are pointing to the store selectors/actions in the tokens i.e: In this case, an This will mean moving the examples to the corresponding location in |
gziolo
reviewed
Jul 22, 2022
Comment on lines
836
to
837
| select( blocksStore ).getCategories() | ||
| ); |
There was a problem hiding this comment.
One more place to fix in the file with original code:
Suggested change
| select( blocksStore ).getCategories() | |
| ); | |
| select( blocksStore ).getCategories(), | |
| [] | |
| ); |
There was a problem hiding this comment.
Ah! Yes, I added that to the actions.js and didn't update it. Updated now.
gziolo
approved these changes
Jul 22, 2022
See my comment #42572 (comment). It's more nuanced. It all depends on the context where you use the code. Most of those wrappers around |
|
@gziolo that makes more sense and thanks for the context there. I think it's probably best to leave things as they are then. |
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?
This PR adds
@exampletags for all non-deprecated, non-experimental selectors for the@wordpress/blockspackage. Part of the effort in #42125.