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

Components: Extract a reusable Post Format Selector #3108

Merged
merged 4 commits into from
Oct 27, 2017

Conversation

youknowriad
Copy link
Contributor

Related to #2761 (comment)

All these (the sidebar panels) must be refactored slightly to drop all the "layout" pieces and keep only the "forms". For example, the sidebar panels like PostTaxonomies, we should extract only the PostTaxonomyForms into a reusable component without the expandable panel behavior that is specific to our current UI. (This step could be done in a separate PR preceding the directory structure change)

This is the first one of multiple PRs addressing the comment above and extracting reusable pieces from our current sidebar panels. This PR does so for the PostFormat component.

Testing instructions

  • Test that the post format selector still works and shows as expected.

@youknowriad youknowriad self-assigned this Oct 23, 2017
@youknowriad youknowriad requested a review from aduth October 23, 2017 08:32
];

function PostFormat( { postType, onUpdatePostFormat, postFormat = 'standard', suggestedFormat, instanceId } ) {
function PostFormat( { postType } ) {
if ( ! get( postType.data, [ 'supports', 'post-formats' ] ) ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hesitated whether to keep this in the reusable component. Even if we do so, we'd have to duplicate it here to avoid rendering the container panel as well. Thoughts @aduth ?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, if the reusable component is still "post aware" it seems to make sense to also be post post-type aware, and reasonable enough to render nothing if the post type does not support. That it's duplicated seems okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth applying the same principle as with PostStickyCheck in #3114?

@codecov
Copy link

codecov bot commented Oct 23, 2017

Codecov Report

Merging #3108 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3108      +/-   ##
==========================================
- Coverage   31.58%   31.51%   -0.08%     
==========================================
  Files         219      221       +2     
  Lines        6291     6305      +14     
  Branches     1118     1121       +3     
==========================================
  Hits         1987     1987              
- Misses       3618     3629      +11     
- Partials      686      689       +3
Impacted Files Coverage Δ
editor/post-format/check.js 0% <0%> (ø)
editor/sidebar/post-format/index.js 0% <0%> (ø) ⬆️
editor/post-format/index.js 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9a05b2...4a2059f. Read the comment docs.

const postFormatSelectorId = 'post-format-selector-' + instanceId;
const suggestion = find( POST_FORMATS, ( format ) => format.id === suggestedFormat );

// Disable reason: A select with an onchange throws a warning
Copy link
Member

Choose a reason for hiding this comment

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

Existed previously but... "it's a warning" is not a reason to disable a warning.

];

function PostFormat( { postType, onUpdatePostFormat, postFormat = 'standard', suggestedFormat, instanceId } ) {
function PostFormat( { postType } ) {
if ( ! get( postType.data, [ 'supports', 'post-formats' ] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, if the reusable component is still "post aware" it seems to make sense to also be post post-type aware, and reasonable enough to render nothing if the post type does not support. That it's duplicated seems okay.

@youknowriad youknowriad requested review from mcsf and gziolo October 26, 2017 09:59
Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

const postFormatSelectorId = 'post-format-selector-' + instanceId;
const suggestion = find( POST_FORMATS, ( format ) => format.id === suggestedFormat );

// Disable reason: We need to change the value immiediately to show/hide the suggestion if needed
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo at immiediately :)

}

const postFormatSelectorId = 'post-format-selector-' + instanceId;
const suggestion = find( POST_FORMATS, ( format ) => format.id === suggestedFormat );
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip: lodash#find allows find( xs, { id: suggestedFormat } ) syntax — unless you want to keep as-in to keep it closer to the Array#find standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, not sure where, but I think I've read that this syntax might be deprecated in lodash 5

];

function PostFormat( { postType, onUpdatePostFormat, postFormat = 'standard', suggestedFormat, instanceId } ) {
function PostFormat( { postType } ) {
if ( ! get( postType.data, [ 'supports', 'post-formats' ] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth applying the same principle as with PostStickyCheck in #3114?

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.

3 participants