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

Introduce theme_supports with formats to REST API index #6318

Merged
merged 6 commits into from
Apr 25, 2018

Conversation

danielbachhuber
Copy link
Member

Merges into #6301

@danielbachhuber danielbachhuber added this to the 2.8 milestone Apr 20, 2018
@danielbachhuber
Copy link
Member Author

@pento One implementation nuance to be aware of: if I create a Post with format aside, then switch to a theme that doesn't support aside, the REST API will return my Post resource as format aside. This is why there's code to ensure supportedFormats always includes the currently selected format. As a byproduct, it also helps mitigate the "flash of loading data" effect.

Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

Left a couple of comments.

I'm not wild about the theme_supports name, but let's go with it for now, we can bikeshed over it later. If someone felt like building an endpoint similar to WPCOM's themes/mine, we could potentially move this info to there.

// Ensure current format is always in the set.
// The current format may not be a supported format.
supportedFormats.push( format );
supportedFormats = uniq( supportedFormats );
Copy link
Member

Choose a reason for hiding this comment

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

Rather than the multi-step process for making supportedFormats unique, we can use the same pattern as we use in the PHP:

const supportedFormats = union( [ format ], get( themeSupports, 'formats', [] ) );

Copy link
Member Author

Choose a reason for hiding this comment

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

$result = $response->get_data();
$this->assertTrue( isset( $result['theme_supports'] ) );
$this->assertTrue( isset( $result['theme_supports']['formats'] ) );
$this->assertTrue( in_array( 'standard', $result['theme_supports']['formats'] ) );
Copy link
Member

Choose a reason for hiding this comment

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

It'd be good to test that post formats that aren't specified by the theme are not in the results, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given post formats can vary from theme to theme, and the theme active in the test suite is uncertain, can you suggest what you'd like me to test?

Copy link
Member

Choose a reason for hiding this comment

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

On second thoughts, I don't know that it needs testing. I'd been looking at WPCOM's API, which tries to avoid loading the theme code if it can, so was considering how to test for that. But, that's not really an issue for Core.

*/
export async function* getThemeSupports() {
const index = await apiRequest( { path: '/' } );
yield receiveIndex( index, 'theme_supports' );
Copy link
Member

Choose a reason for hiding this comment

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

Second argument is unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in 82bdfb5

* @return {Mixed?} Index data.
*/
export function getThemeSupports( state ) {
return state.indexData[ 'theme_supports' ];
Copy link
Member

Choose a reason for hiding this comment

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

Why mixed square bracket notation? Is it because ESLint is throwing an error?

There's a question whether we should model our state shape how we want to use it in the client, i.e. camel-casing properties from the REST API. Less consistent with API properties, more consistent with JS standards. Not sure what the best option is here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why mixed square bracket notation? Is it because ESLint is throwing an error?

Copy-pasta from above. I've changed in d64f40a

return {
postFormat: getEditedPostAttribute( 'format' ),
supportedFormats: get( postType, [ 'formats' ], [] ),
supportedFormats: supportedFormats,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

export default combineReducers( {
terms,
media,
postTypes,
indexData,
Copy link
Member

Choose a reason for hiding this comment

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

Is Data suffix adding any value here? Would we want to call other reducers termsData, mediaData as well for consistency's sake?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is Data suffix adding any value here?

For me personally, it denotes that it's the data part of the index. index was too nebulous in my opinion (in comparison to terms, which more clearly communicates itself).

Would we want to call other reducers termsData, mediaData as well for consistency's sake?

I don't think so. However, if you're not a fan of indexData, I'm open to renaming it. Alternatively, we could only put themeSupports in the store, instead of the entire index.

Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

👍🏻 Let's land this in add/post-format-filtering, and continue there.

@danielbachhuber danielbachhuber merged commit 4f53ba5 into add/post-format-filtering Apr 25, 2018
@danielbachhuber danielbachhuber deleted the post-format-index branch April 25, 2018 12:13
danielbachhuber pushed a commit that referenced this pull request Apr 25, 2018
* Add supported post formats to the post type REST API response

* Filter the post formats we show in the PostFormat dropdown

* Need to pass the slug to post_type_supports()

* Standard post format is added in the end point response, no need to manually add it to the list

* Introduce `theme_supports` with `formats` to REST API index (#6318)

* Introduce `theme_supports` with `formats` to REST API index

* Load supported formats from index into scope

* Use `union` for a more concise logic declaration.

* Remove unused second argument

* Directly access the object variable

* Use shorthand format

* More consise variable definition

* Remove `permalink_structure` definition, which has been removed

* Focus state management on themeSupports data

* Use lodash implementation of `includes()` for IE11 compat
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants