Skip to content
This repository was 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
Alex B (bmdalex) merged 9 commits into
masterfrom
fix/factories-element-props
Dec 10, 2018
Merged

Alex B (bmdalex) merged 9 commits into
masterfrom
fix/factories-element-props

Conversation

@bmdalex

@bmdalex Alex B (bmdalex) commented Nov 19, 2018 •

Copy link
Copy Markdown
Collaborator

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

codecov Bot commented Nov 19, 2018 •

Copy link
Copy Markdown

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.

@bmdalex
Alex B (bmdalex) force-pushed the fix/factories-element-props branch from 3fc758c to 595c817 Compare November 19, 2018 21:31
Comment thread test/specs/lib/factories-test.tsx Outdated
Comment thread src/lib/isElementTypeOfComponent.ts Outdated
Comment thread src/lib/isElementTypeOfComponent.ts Outdated
Comment thread src/lib/isElementTypeOfComponent.ts Outdated
Comment thread src/lib/factories.tsx Outdated
Comment thread src/lib/reactComponentUtils.ts Outdated
Comment thread src/lib/reactComponentUtils.ts Outdated
Comment thread test/utils/createMockComponent.tsx Outdated
Comment thread test/specs/lib/reactComponentUtils-test.tsx Outdated
Comment thread test/specs/lib/factories-test.tsx Outdated
Comment thread test/specs/lib/reactComponentUtils-test.tsx Outdated
Comment thread test/specs/lib/reactComponentUtils-test.tsx Outdated
Comment thread test/specs/lib/reactComponentUtils-test.tsx Outdated

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Collaborator Author

marking blocked until having a resolution on:

#496 (comment)

@bmdalex Alex B (bmdalex) added needs author changes Author needs to implement changes before merge 💥 blocked and removed 🚀 ready for review labels Nov 28, 2018
@bmdalex
Alex B (bmdalex) force-pushed the fix/factories-element-props branch 2 times, most recently from f1d4047 to 9af4c9e Compare December 4, 2018 15:13
Comment thread test/specs/components/RadioGroup/RadioGroup-test.tsx Outdated
Comment thread test/specs/components/RadioGroup/RadioGroup-test.tsx Outdated
@bmdalex
Alex B (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
Copy Markdown
Contributor

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 👍

Comment thread docs/src/examples/components/RadioGroup/Item/RadioGroupItemExample.shorthand.tsx Outdated
Comment thread src/lib/factories.tsx Outdated
Comment thread src/lib/factories.tsx Outdated
Comment thread test/specs/components/RadioGroup/RadioGroup-test.tsx Outdated
Comment thread test/specs/components/RadioGroup/RadioGroup-test.tsx Outdated
Comment thread test/specs/lib/factories-test.tsx Outdated
@bmdalex
Alex B (bmdalex) force-pushed the fix/factories-element-props branch from 6714a85 to e76bf82 Compare December 10, 2018 11:27
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