Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(factories): shorthand fix for applying props to react element #496

Merged
merged 9 commits into from
Dec 10, 2018

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Nov 19, 2018

createShorthandFactory fix for applying props to React Element

Description

This PR tackles the first argument type of the function returned by createShorthandFactory factory, more precisely when passing a React.Element as argument.

This comes into play in examples such as:

<Button
  icon={<span name='coffee' style={{ background: 'red' }}>span content</span>}
  content="Click Me"
/>
<Button
  icon={<Icon name='coffee' style={{ background: 'red' }}>icon content</Icon>}
  content="Click Me"
/>

that translate into calling:

createShorthandFactory(Icon, 'name').create(icon, {
  defaultProps: {
    styles: styles.icon,
    xSpacing: !content ? 'none' : iconPosition === 'after' ? 'before' : 'after',
    variables: variables.icon,
  }
})

// with icon being
<span name='coffee' style={{ background: 'red' }}>span content</span>

// and respectively:
<Icon name='coffee' style={{ background: 'red' }}>icon content</Icon>

and rendering:

screenshot 2018-11-19 at 21 07 16

The icon argument can be any React.Element so we cannot make too many presumptions on what to do with the props send as the 2nd argument (defaultProps and overrideProps).

Current Behavior:

We blindly send all props to the React.cloneElement call (see createShorthand call in factories.tsx).
For the example above, this can lead to passing invalid props to certain components and we currently catch this wrong usage in one of our unit tests:
screenshot 2018-11-19 at 19 34 10

Solution:

If the first argument of the function returned by createShorthandFactory factory is a React.Element just return the element early without cloning and applying extra props.

@codecov
Copy link

codecov bot commented Nov 19, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@0d969eb). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #496   +/-   ##
=========================================
  Coverage          ?   88.32%           
=========================================
  Files             ?       42           
  Lines             ?     1456           
  Branches          ?      212           
=========================================
  Hits              ?     1286           
  Misses            ?      165           
  Partials          ?        5

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 0d969eb...0340cd7. Read the comment docs.

@layershifter layershifter added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Nov 20, 2018
src/lib/isElementTypeOfComponent.ts Outdated Show resolved Hide resolved
src/lib/isElementTypeOfComponent.ts Outdated Show resolved Hide resolved
src/lib/factories.tsx Outdated Show resolved Hide resolved
src/lib/reactComponentUtils.ts Outdated Show resolved Hide resolved
src/lib/reactComponentUtils.ts Outdated Show resolved Hide resolved
test/utils/createMockComponent.tsx Outdated Show resolved Hide resolved
test/specs/lib/reactComponentUtils-test.tsx Outdated Show resolved Hide resolved
test/specs/lib/factories-test.tsx Outdated Show resolved Hide resolved
test/specs/lib/reactComponentUtils-test.tsx Outdated Show resolved Hide resolved
test/specs/lib/reactComponentUtils-test.tsx Outdated Show resolved Hide resolved
test/specs/lib/reactComponentUtils-test.tsx Outdated Show resolved Hide resolved
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

Can we check my solution for areTypeNamesEqual? I think that the check of displayName is not the best solution.

@bmdalex
Copy link
Collaborator Author

bmdalex commented Nov 28, 2018

marking blocked until having a resolution on:

#496 (comment)

@bmdalex bmdalex added needs author changes Author needs to implement changes before merge 💥 blocked and removed 🚀 ready for review labels Nov 28, 2018
@bmdalex bmdalex force-pushed the fix/factories-element-props branch 2 times, most recently from ece34d1 to 6714a85 Compare December 6, 2018 17:07
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

We discussed these changes before, the final result LGTM 👍

src/lib/factories.tsx Outdated Show resolved Hide resolved
src/lib/factories.tsx Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants