Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(styles): expand css shorthands in order for them to be applied properly #869

Merged
merged 30 commits into from
Apr 29, 2019

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Feb 8, 2019

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:

  font
  padding
  margin
  border
  borderWidth
  borderStyle
  borderColor
  borderTop
  borderRight
  borderBottom
  borderLeft
  borderRadius
  background
  outline

This means that we still don't have any mechanism for expanding the other shorthands like:

  animation
  column-rule
  columns flex
  flex-flow grid
  grid-area
  grid-column
  grid-row
  grid-template
  list-style
  offset
  overflow
  place-content
  place-items
  place-self
  text-decoration
  transition

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

  1. Let's say that we have a component where inside the styles prop, we are providing some extended vs shorthand styles
<Segment content="First Segment" styles={{marginBottom: '10px', margin: '0px'}} />

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.

  1. We have components that in their styles have usages of marginRight, marginLeft etc. If the user specify in the styles of this component (or in some nested Provider), that they want margin: 0px this should override the above specified marginRight | Left.
    In the next example we have the Icon component, that by default has right margin of 8px. Using this should override that margin.
<Icon name="call-video" styles={{margin: '0px'}} />

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.

@codecov
Copy link

codecov bot commented Feb 11, 2019

Codecov Report

Merging #869 into master will increase coverage by 0.07%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/react/src/lib/felaRenderer.tsx 75% <ø> (ø) ⬆️
...c/themes/teams/components/Segment/segmentStyles.ts 14.28% <0%> (-2.39%) ⬇️
...ges/react/src/lib/felaExpandCssShorthandsPlugin.ts 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 06f785a...b14b8ae. Read the comment docs.

Copy link
Collaborator

@bmdalex bmdalex left a 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

manajdov added 2 commits February 26, 2019 18:24
@DustyTheBot
Copy link
Collaborator

DustyTheBot commented Feb 26, 2019

Warnings
⚠️ Package (or peer) dependencies changed. Make sure you have approval before merging!

Changed dependencies in packages/react/package.json

package before after
css-shorthand-expand - ^1.2.0
fast-memoize - ^2.5.1

Generated by 🚫 dangerJS

-added test
Copy link
Member

@layershifter layershifter left a 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 📝

Copy link
Member

@layershifter layershifter left a 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 👍

@mnajdova mnajdova merged commit a1b4c59 into master Apr 29, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/expading-css-shorthands branch April 29, 2019 09:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS shorthand properties do not work well when styles are overwritten
5 participants