-
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
Annotations: Replace store name string with exposed store definition #28156
Conversation
Size Change: -309 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
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.
Thank you, looks good 👍
'core/annotations', | ||
addAnnotationClassName | ||
); | ||
addFilter( 'editor.BlockListBlock', STORE_NAME, addAnnotationClassName ); |
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 one is probably incidentally the same. It should remain hardcoded since it need to always remain the same fir backward compatibility.
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.
Sorry for taking so long to answer. Shouldn't it be ok, as the change was using the store name (constant) instead of the definition?
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 fixed it, conceptually those are 2 different things.
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 mean that this core/annotations
reference shouldn't be replaced since it's not related to the store definition?
If that's the case, then I'm sorry. I didn't go further on the function to check whether it was related or not, just replaced it. Sorry :(
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.
👍🏻
Description
Addresses #27088. Replaces the store names (hardcoded strings) with the exposed
store
definitions.There's still (only) one hardcoded reference to the store name left:
e2e-tests/plugins/plugins-api/annotations-sidebar.js
, but since it's a E2E test file, it's best to leave it there, as explained in #27414 (comment).How has this been tested?
npm run test
.Types of changes
Code refactoring, non-breaking change.
Checklist: