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

Implemented HoC ifPropsVerify and IfPostTypeSupports. #4812

Closed

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Feb 1, 2018

ifPropsVerify HoC was implemented, it will be useful in a considerable amount of places. IfPostTypeSupports HoC is a specialization of ifPropsVerify The IfPostTypeSupports replaces PostTypeSupportCheck component, using an HoC makes things easier to test as we don't need to declare more than one component per file as happened in PageAttributesOrder.
A replacement of IfPostTypeSupports was added here as a sample. Provided the new HoC's are approved and merged, all PostTypeSupportCheck and (similar components) will be replaced with HoC's.

Documentation by @aduth:
Example Before:

function MyFeaturedImage() {
	return (
		<PostTypeSupportCheck supportKeys="thumbnail">
			<img />
		</PostTypeSupportCheck>
	);
}

export default connect( /* ... */ )( MyFeaturedImage );

Example After:

function MyFeaturedImage() {
	return <img />;
}

export default compose( [
	ifPostTypeSupports( 'thumbnail' ),
	connect( /* ... */ ),
] )( MyFeaturedImage );

Implementation notes:

The thinking behind the naming convention is that we could create higher-order component-libraries with the following expectations:

Name Behavior
ifSomeCondition Renders component only if condition applies
withSomeProps Renders component with additional props

Testing instructions:

Verify that there are no regressions in the behavior of post-type-conditional elements:

  • Discussion (comments or trackbacks supports)
  • Featured image (thumbnail support)
  • Excerpt (excerpt support)
  • Post format (post-formats support)
  • Author (author support)
  • Order (page-attributes support)
  • Last Revision (revisions support)

@jorgefilipecosta jorgefilipecosta self-assigned this Feb 1, 2018
@jorgefilipecosta jorgefilipecosta added the [Type] Enhancement A suggestion for improvement. label Feb 1, 2018
* @param {string|Array} supportKeys An array or string The component is rendered
* if post type supports at least one of the supports keys passed.
* @returns {Component} Wrapped component.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Name should start with lowercase to match other HOCs. Why not start the name with ‘with’ instead of ‘if’?

@gziolo
Copy link
Member

gziolo commented Feb 1, 2018

I expected this HOC can be applied in more than one place. I like the implementation a lot. I’m only worried tgat it temporiraly adds so many lines of code :)

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Feb 2, 2018

Hi @aduth, I merged your PR #4815 with this one I also changed the name of component withConditionalRender to ifPropsVerify to be inline with the naming convention for conditional render.
edit2: It looks like the errors I referred are not related with this changes, after rebasing and making a clean execution, things look fine without errors.

@jorgefilipecosta jorgefilipecosta changed the title Implemented HoC withConditionalRender and IfPostTypeSupports. Implemented HoC ifPropsVerify and IfPostTypeSupports. Feb 2, 2018
jorgefilipecosta and others added 2 commits February 2, 2018 17:23
The IfPostTypeSupports will replace PostTypeSupportCheck, using HoC makes things easier to test as we don't need to declare more than one component per file as happened in PageAttributesOrder.
@jorgefilipecosta jorgefilipecosta force-pushed the update/implemented-if-post-type-supports branch from 7bfe089 to 18699b4 Compare February 2, 2018 17:24
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

@youknowriad As the original author of PostTypeSupportCheck, do you have any thoughts on the alternative pattern proposed here?

Something about the name ifPropsVerify doesn't sit well with me, though I don't have a good alternative (I actually liked withConditionalRender, but understand why it was changed based on my suggested naming convention).

*/
function ifPropsVerify( predicate ) {
return ( OriginalComponent ) => {
const WrappedComponent = ( props ) => predicate && predicate( props ) && <OriginalComponent { ...props } />;
Copy link
Member

Choose a reason for hiding this comment

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

Mixed messaging here: We don't trust the developer to pass a function as predicate, but we do trust them to not return 0 as the result of predicate (I'll leave it to you to experiment with the outcome 😄 ).

Hint: https://codepen.io/aduth/pen/dWwzrL

Copy link
Member

Choose a reason for hiding this comment

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

So the alternative here would be:

predicate && predicate( props ) ? <OriginalComponent { ...props } /> : null

Copy link
Contributor

Choose a reason for hiding this comment

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

or !! predicate && !! predicate( props ) ? <OriginalComponent { ...props } /> depends if you like the !! or not

WrappedComponent.displayName = getWrapperDisplayName( WrappedComponent, 'ifPostTypeSupports' );
return WrappedComponent;
};
}
Copy link
Contributor

@youknowriad youknowriad Feb 6, 2018

Choose a reason for hiding this comment

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

I think I 'd prefer a renderProp where you pass the result of the check because we have use-case where we want to do if else.

<IfPostTypeSupports
  feature="comments" 
  renderIfTrue={ () => <div>Awesome</div> }
  renderIfFalse={ () => <div>Oh No!</div> }
/>

I think the higher order component could still be exposed as a special case though.

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, I agree this would be difficult to implement with higher-order components.

The prop naming doesn't jive with me, and I'd be more inclined toward a function-as-child alternative, but for no particularly strong reasons:

<IfPostTypeSupports feature="comments">
	{ ( isSupported ) => (
		<div>{ isSupported ? 'Awesome' : 'Oh No!' }</div>
	) }
</IfPostTypeSupports>

(Though in this trivial example it does help for reducing code duplication on shared wrappers, although in the real world this would probably be wrapping the <IfPostTypeSupports /> anyways)

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Feb 17, 2018

Choose a reason for hiding this comment

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

Hi @youknowriad, @aduth, I thought a little and I think we can keep the simplicity of HoC and still allow rendering an else condition.
If we use the HoC like:
ifPostTypeSupports( 'thumbnail' ),

The HoC does not render the component, used in most of the cases, it is the version we have right now.

But if we use the HoC like:
ifPostTypeSupports( 'thumbnail', 'isThumbnailSupported' ),
The component is rendered with the additional prop isThumbnailSupported passe. The user of the HoC can implement its if/else logic.

Let me know your thoughts on this approach.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that works pretty well. Would it be the standard usage? Or would there be a default inferred usage that if the second argument isn't specified, the component doesn't render at all if the condition isn't met?†

† And if so, is this potentially confusing in its inconsistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea was that the components don't render at all if the second argument isn't specified. It may be a not easy to guess behavior but if we document it I think users of the component will get familiar with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants