fix(factories): styles defined as func are not working#470
Conversation
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
levithomason and
miroslavstastny
as code owners
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 4Continue 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.
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.
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
| 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.
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.
Agreed, will encapsulate this logic.
There was a problem hiding this comment.
Introduced, please review again.
kuzhelov
left a comment
kuzhelov
left a comment
There was a problem hiding this comment.
it would be great if we'll be able to encapsulate this logic for merging styles, to prevent similar mistakes from happening in future
kuzhelov
added
needs author feedback
3 participants
TODO
This PR fixes #415