Possible fix for counter-bar props error #42
Conversation
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 19 19
Lines 146 148 +2
Branches 16 16
=====================================
+ Hits 146 148 +2
Continue to review full report at Codecov.
|
components/counter-bar/src/index.js
Outdated
| @@ -240,7 +257,7 @@ CounterBar.Item = ({ | |||
| }) => { | |||
| const Wrapper = wrapper.withComponent(component); | |||
| return ( | |||
| <Wrapper active={active} disabled={!score} empty={ !children || children.length === 0 ? 1 : 0 } {...props}> | |||
| <Wrapper active={active ? 1 : 0} disabled={!score} empty={ !children || children.length === 0 ? 1 : 0 } {...props}> | |||
There was a problem hiding this comment.
this fix is wrong - as is the use of a pseudo-boolean for empty here too - it's a bodge that avoids addressing the underling issue and will instead potentially create other issues with other components we may be using through the component prop
the underlying issue here is that we're using emotion to style and emotion by default will pass through props that should be used for styling only on to the underlying component.
what we should be doing is explicitly ensuring that we do not pass through props that we're using for styling.
emotion includes support for providing an option shouldForwardProp - a function to indicate whether a prop should be forwarded or not which accepts the prop name as a string. some info on this can be found here: emotion-js/emotion#655
for our purposes on the TotalWrapper i believe we'd need something like:
shouldForwardProp: prop => ['active', 'empty'].indexOf(prop) === -1
There was a problem hiding this comment.
an example: https://codesandbox.io/s/v3qrkpoj37
There was a problem hiding this comment.
I see what you mean, Steve, they're unnecessarily being passed on. Will take another look at this on train.
There was a problem hiding this comment.
Nice one - this issue is a PITA, nice fix!
#39