-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Docs: example for getSelectedBlock #66108
Conversation
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. |
Size Change: 0 B Total Size: 1.77 MB ℹ️ View Unchanged
|
Flaky tests detected in 2d840af. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11334592229
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks great. Just one note around using the subscribe
} | ||
}; | ||
|
||
wp.data.subscribe( () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need this part it doesn't add much to the example and might be taken as being needed to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without a listener the function wouldn't log anything to the console, the subscribe would be needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but this could also be assigned to a button onClick or run in a useEffect for example.
Looking at some other examples, we don't always show how the implementation should be used - just how to implement the item in question.
That being said, this is a strong opinion loosely held so I'll leave it up to you :) If you decide to keep it can we change the subscribe
syntax to use imports instead of accessing the global?
Great work and keep them coming!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove subscribe is there a preferred way to add a note relating to a couple of use cases. I never thought about using it in an onClick, might be good to show both options in the docs.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add it commented out similar to how it's shown here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add it commented out similar to how it's shown here
That the correct link? Seems unrelated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 this is the correct link https://developer.wordpress.org/block-editor/reference-guides/packages/packages-core-data/#useentityrecord
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added a couple of examples of how the function can be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with a note
I know you approved but I made a followup change so if you could just take a quick peak before I merge that’d be great |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* add: example for getSelectedBlock * change: build docs
What?
Adding an example for getSelectedBlock from the block-editor package
Why?
Because examples are needed as per #42125
How?
Function had no example before