-
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
Filter the Post Format list to only formats that are supported #6301
Conversation
Given #2523, I'm think formats, featured image, and all theme support data might be better suited to live on the API index response. |
I started moving |
I updated #6318 (which merges into this) to include |
* 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
core-data/reducer.js
Outdated
* | ||
* @return {Object} Updated state. | ||
*/ | ||
export function indexData( state = {}, action ) { |
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.
Continuing discussion from #6318 (comment) : That "index" is considered ambiguous doesn't seem to be helped by the addition of a suffix, and makes me wonder if we can better communicate what it is we're tracking. I guess the endpoint itself is properly called the index (source). From some of the comments, we could derive other names like siteIndex
, siteData
, or maybe just site
.
Most worrying to me is that routes
is a very large data shape that we presumably don't really need here.
/** | ||
* Requests theme supports data from the index. | ||
*/ | ||
export async function* getThemeSupports() { |
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.
Elsewhere we seem to be anticipating that index data may be broadly useful, rather than tracking only theme supports. But it's not consistent with how we've implemented resolvers and selectors, as there's only exposed access to theme supports. Scaling this out to other properties will require (maybe not so obviously) refactoring of the resolver to implement an isFulfilled
for all index-sourced values (see #6084).
Which is to say: We should probably be consistent one way or the other: Only track theme supports, or implement resolvers and selectors as broadly index-sourced.
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 think it's better to focus specifically on theme supports data: 1c6e693
core-data/selectors.js
Outdated
* | ||
* @param {Object} state Data state. | ||
* | ||
* @return {Mixed?} Index data. |
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.
Mixed
is not a valid type. JSDoc encourages*
as the wildcard "any" type (reference)- Extra whitespace should be removed after types, both in
@param
and@return
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.
Handled in 1c6e693
const themeSupports = select( 'core' ).getThemeSupports(); | ||
// Ensure current format is always in the set. | ||
// The current format may not be a format supported by the theme. | ||
const supportedFormats = union( [ format ], get( themeSupports, 'formats', [] ) ); | ||
return { | ||
postFormat: getEditedPostAttribute( 'format' ), |
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.
We can leverage the assigned variable format
here.
(And optionally rename as postFormat
for consistency / benefit of object property shorthand)
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.
lib/compat.php
Outdated
* @param WP_REST_Response $response WP REST API response of the wp-json index. | ||
* @return WP_REST_Response Response that contains the permalink structure. | ||
*/ | ||
function gutenberg_ensure_wp_json_has_permalink_structure( $response ) { |
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.
Is the (re-?)introduction of permalink structure intended? Guessing not by #6319.
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.
Nope. I think this was a bad merge conflict. Removed in 8830dd7
const postFormatSelectorId = 'post-format-selector-' + instanceId; | ||
const suggestion = find( POST_FORMATS, ( format ) => format.id === suggestedFormat ); | ||
const formats = POST_FORMATS.filter( ( format ) => supportedFormats.includes( format.id ) ); |
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.
Array#includes
(and any other prototype-member function) is not polyfilled, and this will error in IE11.
Use Lodash _.includes
instead.
See also: #746
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
As described in #6296, the
PostFormat
list should only display post formats that are currently supported.Core Ticket: https://core.trac.wordpress.org/ticket/43817
Screenshots
Twenty Seventeen restricts the post formats than can be used:
Checklist: