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

fix(Card): Wrap in React.forwardRef #26

Merged
merged 1 commit into from
Mar 16, 2019
Merged

Conversation

cehsu
Copy link
Contributor

@cehsu cehsu commented Mar 8, 2019

Description

This change wraps <Card/> in React.forwardRef.

Task

https://app.asana.com/0/887402852146010/1112252089781752/f

@codecov-io
Copy link

Codecov Report

Merging #26 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #26   +/-   ##
=======================================
  Coverage   71.91%   71.91%           
=======================================
  Files          12       12           
  Lines         146      146           
  Branches       19       19           
=======================================
  Hits          105      105           
  Misses         31       31           
  Partials       10       10
Impacted Files Coverage Δ
src/Card/Card.js 100% <100%> (ø) ⬆️

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 045ae6a...2956adc. Read the comment docs.

1 similar comment
@codecov-io
Copy link

codecov-io commented Mar 8, 2019

Codecov Report

Merging #26 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #26   +/-   ##
=======================================
  Coverage   71.91%   71.91%           
=======================================
  Files          12       12           
  Lines         146      146           
  Branches       19       19           
=======================================
  Hits          105      105           
  Misses         31       31           
  Partials       10       10
Impacted Files Coverage Δ
src/Card/Card.js 100% <100%> (ø) ⬆️

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 045ae6a...2956adc. Read the comment docs.

@cehsu cehsu requested a review from a team March 11, 2019 18:27
@@ -16,7 +16,7 @@ const StyledCard = createComponent({
`,
});

const Card = props => <StyledCard {...props} />;
const Card = React.forwardRef((props, ref) => <StyledCard ref={ref} {...props} />);

Choose a reason for hiding this comment

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

styled-components will forward refs for you but if you want the ref to be on Box instead of the wrapped StyledCard you might have to verify that createComponent does the right thing. could be worth adding a test in that case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will add a test to ensure that the ref is assigned as expected.

Copy link
Contributor

@kylealwyn kylealwyn Mar 13, 2019

Choose a reason for hiding this comment

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

This is right - functional components don't forward refs out of the box and this ref should be passed directly to Box. We're wrapping some components as a hack for <PropsTable /> to work with styled-components. All documentation generators (storybook, docz, stylguidist) are affected due to the issue in the underlying react-docgen parser. Searching for better solutions

@kylealwyn kylealwyn merged commit 0a3d976 into master Mar 16, 2019
@kylealwyn kylealwyn deleted the fix/card-forward-ref branch March 16, 2019 00:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants