fix(css-shorthands): expanding the shorthand properties in the styles #296

mnajdova · 2018-10-02T16:47:56Z

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.

NOTE

The css-shorthand-expand library was replaced with a better alternative shortcss. There is one known bug in the first library that is preventing us of using it. I liked the second library better because of more cleaner expanding of the props.

…geTheme logic

kuzhelov · 2018-10-02T17:22:26Z

this is a nice move, great that this one is finally addressed 👍

Speaking of the points raised.

Not all CSS properties are expanded

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

Here I would suggest to search for extensibility options for css-shorthand-expand library (or any other analog that is widely used), including the option of make a contribution to the library's repo. In general would suggest to encapsulate this expansion process in a separate module/function (haven't reviewed PR yet), so it would be easier for us to switch the engine of this functionality (incl. changing the library altogether).

However, considering which properties are not handled by the css-shorthand-expand, it seems that we could progress with css-shorthand-expand library for now, as most of them are not commonly used as mix of short/long forms (except, surely, the overflow one).

Performance considerations

Agree with these concerns, but it seems that if we would be able to make this functionality to be a Fela's plugin (once again, haven't reviewed PR yet), it would be quite easy for us to make perf measurements, due to there would be mechanism to enable and disable this functionality for the overall pipeline. So, for that reason I don't think that we should wait for perf measurements' scaffolding before introducing these changes.

levithomason · 2018-10-02T20:54:31Z

I'm not sure we should add this here, or possibly at all.

Where would it be added?

This is an operation that mutates the style object, so it would be a plugin in the felaRenderer. These are responsible for processing all style objects. Any rendered style object much first pass through the renderer's plugins. This gives a SSOT for handling style object transforms.

If it is added to mergeThemes as in the current PR, then only styles that are passed through mergedThemes will be expanded. This is unsafe as we will expose utilities for consumers to render their styles into class names and they may not first merge those styles.

Should it be added at all?

Simple is better than complex. Predictable is better than magical. Consider this:

http://fela.js.org/docs/introduction/Caveats.html#2-shorthand--longhand-properties

Probably the biggest downside of using atomic CSS design, is the fact that shorthand and longhand properties can't safely be mixed in a single rule.
...
PS: There will soon be a tool, that automatically checks for mixed shorthand / longhand properties and throws a warning if used together.

I think it might possibly be better to simply not allow shorthand mixture with expanded properties. Or, don't allow shorthand properties at all:

Fewer dependencies
Less logic / room for bugs
Less confusing to the user, styles are exactly as they defined

Conclusion

I don't think we should add a separate dependency for this right now. I also think we should use a renderer plugin if we do add something. If we add something, it should probably be an error for now.

Feedback?

brianespinosa · 2018-10-02T22:55:00Z

I don't think we should add a separate dependency for this right now. I also think we should use a renderer plugin if we do add something. If we add something, it should probably be an error for now.

I was thinking about this as I watch all of these PRs fly in. I think a simple validation check on styles to throw a runtime error to the user to let them know they should not mix shorthands would be the best way to go.

It might also be worth reaching out to @rofrischmann and find out how close he may or may not be to that tool he mentions in Fela docs. If it's close to complete, it might be worth just waiting for it and adding it at the felaRenderer level (provided that is the intended implementation of the tool).

kuzhelov · 2018-10-03T11:21:34Z

Let me, please, respond to the points raised.

@levithomason

Where would it be added?

Totally agree that it should be added/used as Fela plugin - have expressed the same vision on that in my previous comment (#296 (comment)).

Should it be added at all? Simple is better than complex. Predictable is better than magical.

To me it seems that it rather should. Agree with all the points cited, but let me dissect the predictable part of it.

We should not 'surprise' client in a bad way

Speaking of this predictability point, we might argue that Stardust should provide functionality (APIs) that would be intuitive both in terms of their usage, as well as in terms of the result of their usage. This moment loudly attributes to the 'Principle of Least Astonishment' - or, in more informal form, "make interfaces easy to use correctly and hard to use incorrectly" (https://www.youtube.com/watch?v=5tg1ONG18H8&t=868s).

With that being said, lets consider the situation described by #237 - essentially, how it looks on the side of the client. Is it a point of astonishment? I would say decent 'yes' - because even if client would follow all the Stardust guidelines, she will still receive rendering result that is

not expected
hard to debug the reason for

@brianespinosa, @levithomason

I think a simple validation check on styles to throw a runtime error to the user to let them know they should not mix shorthands would be the best way to go.

If we add something, it should probably be an error for now.

I might agree that it could be considered a step in the right direction for short-term perspective, but still am not sure whether we could consider this to be a solution for the problem.

Specifically, let me provide the reasons for that.

Show warnings?

Suppose that we will decide to show warnings (so that components tree will be rendered). In that case I might easily imagine the situation where problems won't be noticed by client (once again, we should think about client use cases - where there might not be any screen validation tests) - for example, due to the fact that styles should be applied for some pseudo-state that is not displayed by default. Thus, we won't be able to prevent this problem from happening on the client side - or, better to say, we won't be able to prevent our clients from shipping their product with broken styles.

Throw errors?

Another alternative - throw error and prevent component tree from being rendered. While, for this case, the problem of broken styles for sure will be noticed, I might think about cases where client will decide to consume theme dynamically in her product (e.g. load theme object through ajax requests as one of the examples) - and, in that case it might happen that theme that has been dynamically applied will break the app. To me it seems to be a point of huge astonishment as well - given that styles have broken the entire app, - maybe much worse than the previous one.

Conclusion

So, for these reasons I'd rather see this 'CSS prop expansion' functionality being supported - for the sake of preventing any surprises to our clients.

robinweser · 2018-10-03T12:04:18Z

@mnajdova Thanks for the mail, I'll write here so everyone can see my response =)
First of all, I'm not done yet with that tool yet as I'm mostly using longhand properties anyways. I've seen other people using the fela-plugin-custom-property plugin to expand shorthand properties, but I like the idea of using a library for that.

In my opinion, it's worth adding that to the core as it reduces confusion a lot. I don't think performance is an issue here.
I once had that plugin ready somewhere, might find it later. It basically warns when shorthand and longhand properties are used together in one resolved style object.
I think in most cases, it's an issue with all the border properties. We tend to mix them most often. They're super annoying too because there're 2 levels of shorthands e.g. border as the top level and borderTop, borderLeft, borderRight, borderBottom as well as borderWidth, borderStyle and borderColor as second level shorthands. Actually border: 1px solid grey should correctly resolve to:

border-top-width: 1px;
border-top-style: solid;
border-top-color: grey;
border-right-width: 1px;
border-right-style: solid;
border-right-color: grey;
border-bottom-width: 1px;
border-bottom-style: solid;
border-bottom-color: grey;
border-left-width: 1px;
border-left-style: solid;
border-left-color: grey;

Maybe the plugin in combination with built-in expansion is the way to go? In devMode most mixing issue will be logged and eventually fixed by the developer, but you still have the shorthand expansion in production (especially for dynamic styles).

tl;dr: I'd be more than happy collaborating on both tools as fela plugins. Especially adding the missing props to the expansion library.

mnajdova · 2018-10-03T12:16:54Z

Where would it be added?

This is an operation that mutates the style object, so it would be a plugin in the felaRenderer. These are responsible for processing all style objects. Any rendered style object much first pass through the renderer's plugins. This gives a SSOT for handling style object transforms.

@levithomason be assured that this will be added as a fela plugin, this is just a showcase that it works.

@rofrischmann thanks very much for sharing your thoughts on this PR :)

mnajdova · 2018-10-03T13:07:33Z

The logic for expanding the css shorthands is now moved to plugin. Please, share your feedback.

src/lib/felaExpandCssShorthandsPlugin.ts

kuzhelov · 2018-10-08T12:11:25Z

src/lib/felaExpandCssShorthandsPlugin.ts

+  const expandCssShorthands = styles => {
+    const processedStyles = {}
+
+    Object.keys(styles).forEach(cssPropertyName => {


nit: if lodash is imported and, what is more appealing, is used below, we might consider to use _.keys :) in either case, please, feel free to decide by your own

The code is refactored.

src/lib/felaExpandCssShorthandsPlugin.ts

kuzhelov · 2018-10-08T12:23:11Z

src/lib/felaExpandCssShorthandsPlugin.ts

+  return expandCssShorthands
+}
+
+const shorthands = [


could you, please, suggest which cases we are aim to prevent with this explicit list?

I deleted it

kuzhelov · 2018-10-08T12:23:39Z

src/lib/felaRenderer.tsx

@@ -9,6 +10,8 @@ import { IRenderer } from '../../types/theme'

 const createRendererConfig = (options: any = {}) => ({
  plugins: [
+    felaExpandCssShorthandsPlugin(),


shouldn't it come after CSS is sanitized?

-moved it after the sanitize plugin

codecov · 2018-10-09T08:38:40Z

Codecov Report

Merging #296 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #296   +/-   ##
=======================================
  Coverage   89.59%   89.59%           
=======================================
  Files          62       62           
  Lines        1220     1220           
  Branches      156      156           
=======================================
  Hits         1093     1093           
  Misses        125      125           
  Partials        2        2

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 763973a...3b8453a. Read the comment docs.

…do classes too -added test for the new fela plugin

# Conflicts: # package.json

… not released newer version after it is fixed)

-added tests for covering the borderColor prop (as an example for ensuring two-words properties work)

mnajdova · 2019-04-24T17:43:39Z

Closing this in favor of #869

-added css-shorthand-expand dependency and incorporated it in the mer…

1b01f56

…geTheme logic

mnajdova requested review from dvdzkwsk, kuzhelov, levithomason and miroslavstastny as code owners October 2, 2018 16:47

mnajdova added the question Further information is requested, concerns that require additional thought are raised label Oct 2, 2018

kuzhelov added the 🚀 ready for review label Oct 2, 2018

-introduced the logic for expanding the css shorthands in a fela plugin

e1cc5f8

mnajdova added the RFC label Oct 3, 2018

kuzhelov reviewed Oct 8, 2018

View reviewed changes

src/lib/felaExpandCssShorthandsPlugin.ts Show resolved Hide resolved

kuzhelov reviewed Oct 8, 2018

View reviewed changes

src/lib/felaExpandCssShorthandsPlugin.ts Outdated Show resolved Hide resolved

kuzhelov reviewed Oct 8, 2018

View reviewed changes

kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Oct 8, 2018

-refactored the expand css shorthand fela plugin

bb969c5

-moved it after the sanitize plugin

manajdov added 5 commits October 9, 2018 11:22

-added logic for expanding the shorthand css props in the nested pseu…

6b9b926

…do classes too -added test for the new fela plugin

Merge branch 'master' into bug/css-shorthand

4f695db

# Conflicts: # package.json

-replaced css-shorthand-props with shortcss (because if known bug and…

5c7f583

… not released newer version after it is fixed)

-fixed checking for a shorthand prop

cffae90

-added tests for covering the borderColor prop (as an example for ensuring two-words properties work)

-better description of the tests

3b8453a

kuzhelov added 🚧 WIP and removed needs author feedback Author's opinion is asked question Further information is requested, concerns that require additional thought are raised labels Oct 9, 2018

mnajdova mentioned this pull request Feb 8, 2019

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

Merged

mnajdova closed this Apr 24, 2019

fix(css-shorthands): expanding the shorthand properties in the styles #296

fix(css-shorthands): expanding the shorthand properties in the styles #296

Uh oh!

Conversation

mnajdova commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

1. The library provides extending for the following shorthands:

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.

NOTE

Uh oh!

kuzhelov commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Not all CSS properties are expanded

Performance considerations

Uh oh!

levithomason commented Oct 2, 2018

Where would it be added?

Should it be added at all?

Conclusion

Uh oh!

brianespinosa commented Oct 2, 2018

Uh oh!

kuzhelov commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

We should not 'surprise' client in a bad way

Show warnings?

Throw errors?

Conclusion

Uh oh!

robinweser commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnajdova commented Oct 3, 2018

Where would it be added?

Uh oh!

mnajdova commented Oct 3, 2018

Uh oh!

Uh oh!

kuzhelov Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

mnajdova Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kuzhelov Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

mnajdova Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

kuzhelov Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mnajdova commented Apr 24, 2019

Uh oh!

Uh oh!

mnajdova commented Oct 2, 2018 •

edited

Loading

kuzhelov commented Oct 2, 2018 •

edited

Loading

kuzhelov commented Oct 3, 2018 •

edited

Loading

robinweser commented Oct 3, 2018 •

edited

Loading

codecov bot commented Oct 9, 2018 •

edited

Loading