-
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
Expose getEditedPostAttribute selector #5003
Expose getEditedPostAttribute selector #5003
Conversation
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.
Maybe, getEditedPostAttribute
should call getEditedPostContent
if it is called with 'content' as the second argument. What do you think @youknowriad and @gziolo?
We can also remove |
Unlike others, Whether it should be exposed depends on whether we care to expose this detail to the consumer (i.e. that it has custom logic), or if from a developer's perspective they only care that they receive content and not so much how it is generated. |
@aduth, I was educated by @xyfi about this complex logic of
|
That sounds fine to me. |
I could quite easily implement that in this PR. But I would rather see a separate function to get multiple attributes, which would lead to a cleaner codebase. |
Yes, it makes sense 👍 |
@@ -43,7 +43,7 @@ function FeaturedImage( { isOpened, postType, onTogglePanel } ) { | |||
} | |||
|
|||
const applyQuery = query( ( select ) => ( { | |||
postTypeSlug: select( 'core/editor', 'getCurrentPostType' ), | |||
postTypeSlug: select( 'core/editor', 'getEditedPostAttribute' ), |
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.
type
should be passed as the last argument.
@@ -26,9 +26,7 @@ const { | |||
getCurrentPostLastRevisionId, | |||
getCurrentPostRevisionsCount, | |||
getCurrentPostType, |
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.
Should we also remove this one in the follow-up PR?
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.
select( 'core/editor', 'getEditedPostAttribute', 'type' )
- we use that already in this PR.
Steps to reproduce:
|
Can you explain why this is included? As implemented, it's a bit concerning because it returns a new object on every call, meaning that any |
editor/store/selectors.js
Outdated
*/ | ||
export function getEditedPostAttributes( state, attributeNames ) { | ||
const attributes = {}; | ||
map( attributeNames, attributeName => { |
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 pretty prime candidate for reduce
:
export function getEditedPostAttributes( state, attributeNames ) {
return reduce( attributeNames, ( result, attributeName ) => {
const value = getEditedPostAttribute( state, attributeName );
if ( value ) {
result[ attributeName ] = value;
}
return result;
}, {} );
}
https://lodash.com/docs/4.17.5#reduce
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Reduce
data/index.js
Outdated
@@ -100,10 +100,6 @@ export const query = ( mapSelectorsToProps ) => ( WrappedComponent ) => { | |||
}; | |||
|
|||
return connectWithStore( ( state, ownProps ) => { | |||
const select = ( key, selectorName, ...args ) => { |
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.
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.
It seems like code where I left comment: https://github.com/WordPress/gutenberg/pull/5003/files#r167861481, shouldn't be in this PR. Otherwise it is ready to go. 👍
Thanks for performing this refactoring. I like how flexible this new selector is. Do we have those selectors documented? It would be nice to start promoting them in Gutenberg handbook. |
For: WordPress#5032 Related: WordPress#5003
@gziolo I created #5033 to improve the docs for those selectors - this change, which is really cool, made one of the examples there incorrect. I documented allow the post attributes I could find -post, type, slug and content. Are there others I can use? Am I right to assume that |
We would have to test it, you can check inside the state. I would document only those attributes that we use in Gutenberg at the moment, maybe even add a whitelist to have a better overview what we need to support in the future. |
One issue I have here is that it's my understanding with the split between Currently this is exposed under the I might suggest that we either move forward with migrating more of these selectors to cc @youknowriad |
I don't have a precise idea how this will evolve. I think the refactoring we plan to do for the editor to consider it as a template editor defaulting to |
Description
In #4802 @aduth voiced some concerns if attributes should have dedicated selectors for the
select
API. This PR removes thegetEditedPostSlug
andgetEditedPostTitle
selectors and exposes thegetEditedPostAttribute
selector.I have removed the
getEditedPostContent
selector from the public API but not the selector itself, because it contains additional logic. I added a switch statement togetEditedPostAttribute
to callgetEditedPostContent
when the attribute name passed iscontent
.Initially also added
getEditedPostAttributes
selector this PR, but moved it to a separate branch.add/get-edited-post-attributes-selector
.How Has This Been Tested?
Rewritten existing unit tests.
Console tested.
Screenshots (jpeg or gifs if applicable):
Types of changes
Breaking change, if someone has already used the
getEditedPostSlug
orgetEditedPostTitle
selector.Checklist: