Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented Aug 22, 2017

Closes #2367

This pull request seeks to use post types support to determine whether "Post Formats" should be used. It uses the recently-introduced withAPIData higher-order component (#1974) to retrieve post types support from the REST API.

Open questions:

By default, the field will be hidden until the post type has been requested from the REST API. We could optimistically default to the shown case instead, hiding the field if only the post type does not support post-formats.

This may be an area where we want to optimize for the common case:

  • Keep currently behavior, but hard-code post type as assuming to support post formats (unless response reports otherwise)
  • Alternatively, we may consider enhancing the API data retrieval to bootstrap common needs

Testing instructions:

Verify that post formats field shows on post types (assuming theme does not override default), but not on post types without support (typically pages, or custom post types).

@aduth aduth added the General Interface Parts of the UI which don't fall neatly under other labels. label Aug 22, 2017
@aduth aduth requested a review from youknowriad August 22, 2017 18:26
@codecov
Copy link

codecov bot commented Aug 22, 2017

Codecov Report

Merging #2500 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2500      +/-   ##
==========================================
- Coverage   28.65%   28.64%   -0.02%     
==========================================
  Files         165      165              
  Lines        5039     5041       +2     
  Branches      830      830              
==========================================
  Hits         1444     1444              
- Misses       3052     3054       +2     
  Partials      543      543
Impacted Files Coverage Δ
editor/sidebar/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 67e759b...feaad79. Read the comment docs.

* External dependencies
*/
import { connect } from 'react-redux';
import { find } from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we're using flowRight in Calypso, but I don't see any reason to not just use flow instead. I know compose aka flowRight is more widely used in the community.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm actually a bit confused by the behavior if I'm honest. I'd wanted to define HoCs from left-to-right, which is not the functional pattern, and is what I understand flow to achieve. But in practice this was being reversed, and the props added from the left-most HoC (connect) weren't reaching withAPIData.

So, I agree on the behavior of defining HoCs, but the use of flow appears to be backwards. I might toy with this a bit to see why it's this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I think I understand it better now. It is because of how HoCs wrap the original component:

flow( [ connect, withAPIData ] )( Component ) is equivalent to:

<WithAPIDataComponent>
    <ConnectedComponent>
        <Component />
    </ConnectedComponent>
</WithAPIDataComponent>

In this case, props from connect props are not available to the withAPIData component, because the latter is the parent.

flowRight( [ connect, withAPIData ] )( Component ) is equivalent to:

<ConnectedComponent>
    <WithAPIDataComponent>
        <Component />
    </WithAPIDataComponent>
</ConnectedComponent>

In this case, props from connect props are available to the withAPIData component, because the latter is the child.

So while in a normal case you'd expect flow functions to each use the resulting value from the previous function invocation, this is reversed in the case of higher-order components.

Copy link
Member

Choose a reason for hiding this comment

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

When using flowRight / compose you have the order which mirrors how components are wrapped. That might help to make it easier to read:

flowRight(
    connect,
        withAPIData
)(
            Component
);

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Works like a charm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

General Interface Parts of the UI which don't fall neatly under other labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants