-
Notifications
You must be signed in to change notification settings - Fork 55
fix(factories): styles defined as func are not working #470
Conversation
Codecov Report
@@ Coverage Diff @@
## master #470 +/- ##
=======================================
Coverage 88.96% 88.96%
=======================================
Files 41 41
Lines 1387 1387
Branches 177 202 +25
=======================================
Hits 1234 1234
Misses 149 149
Partials 4 4 Continue to review full report at Codecov.
|
test('deep merges overrideProps styles as function onto styles prop', () => { | ||
expect.assertions(1) | ||
|
||
const overrideProps = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be a bit easier to reason about expected results if these two props
objects will be swapped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather leave this tests the same as the previous ones for consistency, as the only difference is changing the objects to functions.
src/lib/factories.tsx
Outdated
@@ -106,7 +107,13 @@ export function createShorthand( | |||
|
|||
// Merge styles | |||
if (defaultProps.styles || overrideProps.styles || usersProps.styles) { | |||
props.styles = _.merge(defaultProps.styles, usersProps.styles, overrideProps.styles) | |||
props.styles = (...args) => { | |||
return _.merge( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we want to have mergeStyles
function that would encapsulate this logic? Similar semantics is expressed in the mergeThemes
file:
// mergeComponentStyles() {
...
return _.merge(callable(originalTarget)(styleParam), callable(originalSource)(styleParam))
maybe we could encapsulate and reuse this logic in the following way:
mergeStyles(target: ComponentSlotStyle, ...styles: ComponentSlotStyle[]) {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, will encapsulate this logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced, please review again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be great if we'll be able to encapsulate this logic for merging styles, to prevent similar mistakes from happening in future
TODO
This PR fixes #415