Skip to content
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

Merged
merged 10 commits into from
Feb 13, 2018
Merged

Expose getEditedPostAttribute selector #5003

merged 10 commits into from
Feb 13, 2018

Conversation

abotteram
Copy link
Contributor

@abotteram abotteram commented Feb 12, 2018

Description

In #4802 @aduth voiced some concerns if attributes should have dedicated selectors for the select API. This PR removes the getEditedPostSlug and getEditedPostTitle selectors and exposes the getEditedPostAttribute 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 to getEditedPostAttribute to call getEditedPostContent when the attribute name passed is content.

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 or getEditedPostTitle selector.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@abotteram abotteram changed the title Expose get edited post attribute Expose getEditedPostAttribute selector Feb 12, 2018
Copy link
Member

@atimmer atimmer left a 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?

@gziolo
Copy link
Member

gziolo commented Feb 12, 2018

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 getEditedPostContent as it seems to be obsolete with the new unified approach. Just guessing, I haven't checked the actual code :)

@aduth
Copy link
Member

aduth commented Feb 12, 2018

Unlike others, getEditedPostContent is not simply a property accessor proxy and has its own logic: Specifically that it serializes blocks in most cases.

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.

@gziolo
Copy link
Member

gziolo commented Feb 12, 2018

@aduth, I was educated by @xyfi about this complex logic of getEditedPostContent, but I still think that it makes sense to have one selector that knows how to return all types of attributes. As a plugin developer, I shouldn't be forced to learn that those attributes are handled differently. In the future, we can also make their life easier if we decide to support returning a group of attributes like this:

const applyQuery = query( ( select ) => ( {
	...select( 'core/editor', 'getEditedPostAttribute', [ 'title', 'content', 'slug' ] ),
} ) );

@aduth
Copy link
Member

aduth commented Feb 12, 2018

That sounds fine to me.

@abotteram
Copy link
Contributor Author

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.

@gziolo
Copy link
Member

gziolo commented Feb 12, 2018

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' ),
Copy link
Member

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,
Copy link
Member

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?

Copy link
Member

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.

@aduth
Copy link
Member

aduth commented Feb 12, 2018

This might be related to the failing tests 😅

image

@aduth
Copy link
Member

aduth commented Feb 12, 2018

Steps to reproduce:

  1. Try switching to Text mode

@aduth
Copy link
Member

aduth commented Feb 12, 2018

Also adds the getEditedPostAttributes selector for getting multiple post attributes.

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 connected component which uses it would re-render on every change to state. Could be memoized, or dropped altogether (What's the use case? Could a developer just call the singular form with individual attributes instead?)

*/
export function getEditedPostAttributes( state, attributeNames ) {
const attributes = {};
map( attributeNames, attributeName => {
Copy link
Member

@aduth aduth Feb 12, 2018

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 ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this code removed purposefully? I mean @aduth removes it with #5007, too. However not sure if that should be a part of this PR.

Copy link
Member

@gziolo gziolo left a 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. 👍

@gziolo gziolo merged commit 7006ee1 into WordPress:master Feb 13, 2018
@gziolo
Copy link
Member

gziolo commented Feb 13, 2018

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.

@gziolo gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Feb 13, 2018
@atimmer atimmer deleted the add/expose-get-edited-post-attribute branch February 13, 2018 14:21
Shelob9 added a commit to Shelob9/gutenberg that referenced this pull request Feb 13, 2018
@Shelob9
Copy link
Contributor

Shelob9 commented Feb 13, 2018

@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 wp.data.select( 'core/editor', 'getEditedPostAttribute', 'excerpt' ) would provide the post excerpt? I can't figure out -- but also, I'm not fully caffeinated -- if any post attributes are available via that selector or only a specific white list. I'd like to improve that example.

@gziolo
Copy link
Member

gziolo commented Feb 14, 2018

Am I right to assume that wp.data.select( 'core/editor', 'getEditedPostAttribute', 'excerpt' ) would provide the post excerpt? I can't figure out -- but also, I'm not fully caffeinated -- if any post attributes are available via that selector or only a specific white list. I'd like to improve that example.

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.

@aduth
Copy link
Member

aduth commented Feb 15, 2018

One issue I have here is that it's my understanding with the split between editor and edit-post (#4661) that we'll want all post-specific behaviors (including selectors) to live in edit-post.

Currently this is exposed under the editor module key, and I expect this will break in a future release.

I might suggest that we either move forward with migrating more of these selectors to edit-post, or artificially expose them in the correct namespace (i.e. keep in editor/store/index.js, but call registerSelectors with 'core/edit-post' as the namespace for selectors we know to be post-specific).

cc @youknowriad

@youknowriad
Copy link
Contributor

might suggest that we either move forward with migrating more of these selectors to edit-post, or artificially expose them in the correct namespace (i.e. keep in editor/store/index.js, but call registerSelectors with 'core/edit-post' as the namespace for selectors we know to be post-specific).

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 post title - post content and making the editor generic enough to handle editing with or without post might have an impact on the need or the shape of these selectors. So my personal opinion for now would be to just stick with what we have right now but granted that this will create some backwards compatibility burden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants