-
Notifications
You must be signed in to change notification settings - Fork 53
fix(styles): expand css shorthands in order for them to be applied properly #869
Conversation
Codecov Report
@@ Coverage Diff @@
## master #869 +/- ##
==========================================
+ Coverage 72% 72.07% +0.07%
==========================================
Files 734 735 +1
Lines 5608 5626 +18
Branches 1640 1624 -16
==========================================
+ Hits 4038 4055 +17
- Misses 1565 1566 +1
Partials 5 5
Continue to review full report at Codecov.
|
# Conflicts: # CHANGELOG.md
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.
yuppieee 👍 few comments but looks good
# Conflicts: # CHANGELOG.md # package.json # yarn.lock
Changed dependencies in
Generated by 🚫 dangerJS |
-added test
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.
As this PR introduces a new plugin for Fela I suggest to measure performance before merge 📝
-refactorings
# Conflicts: # CHANGELOG.md # packages/react/package.json # yarn.lock
# Conflicts: # CHANGELOG.md
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.
We spent some time to make it more performant, so now it's quite fast 👍
Picks changes from #296 Fixes #237 Please see additional conversation on this PR #296
Fix
This PR aims to fix #237 by expanding the css shorthands of the theme's components styles and the styles prop provided to the component while they are merged.
I added the css-shorthand-expand library and provided service for expanding the styles. There are still some points that should be addressed:
1. The library provides extending for the following shorthands:
This means that we still don't have any mechanism for expanding the other shorthands like:
2. It would have been nice if we would be able to measure the performance of the components and conclude whether this changes would impact them.
I need input whether this is worth exploring further, or whether maybe we should just try to use the library when defining the styles for the themes or applying the styles prop on the components.
Examples which are fixed with this
The example is simple, but the
margin: '0px'
can be provided based on some condition, so in some cases it would make sense.The expected result is that the
margin: '0px'
will override the marginBottom. This PR insures that.marginRight
,marginLeft
etc. If the user specify in thestyles
of this component (or in some nestedProvider
), that they wantmargin: 0px
this should override the above specifiedmarginRight | Left
.In the next example we have the
Icon
component, that by default has right margin of8px
. Using this should override that margin.The prove that this didn't work before is shown in the screener changes for the icon used inside the radio item component. Basically the
radioItemGroupStyles
is overriding the right margin of the icon with the styles for the icon's slot in the following manner:margin: '0 10px 0 0'
, but this style was never applied, and we were not even aware that the right margin was not overriden.