cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.06k stars 397 forks source link

[jss-plugin-expand] Border not expanding correctly #1323

Open YassienW opened 4 years ago

YassienW commented 4 years ago

Expected behavior:

As per https://cssinjs.org/jss-plugin-expand?v=v10.0.0-alpha.9

container: {
    border: {
      color: 'black',
      width: 1,
      style: 'solid'
    }
  }

should compile to

.container {
  border: 1px solid black;
}

Describe the bug:

It compiles to

.container {
  border-width: 1px;
  border-style: solid;
  border-color: black;
}

Codesandbox link: https://codesandbox.io/s/jss-plugin-expand-issue-ww682

Versions (please complete the following information):

This was briefly mentioned in #1023

Edit: Same issue with font properties

kof commented 3 years ago

Why do you consider this to be a bug? This is valid CSS

.container {
  border-width: 1px;
  border-style: solid;
  border-color: black;
}
YassienW commented 3 years ago

I thought a one liner would be expected and is a feature of this plugin as per the documentation: https://cssinjs.org/jss-plugin-expand?v=v10.0.0-alpha.9

If not i guess the documentation should be updated for consistency, i wasn't entirely sure if i was doing something wrong or if the documentation was incorrect

kof commented 3 years ago

yeah, docs is wrong there, though I don't remember exactly why we don't generate a shortcut in this case, maybe @typical000 remembers

typical000 commented 3 years ago

As far as I remember the reason was that it's more "safe" and optimal in terms of performance to write all rules separately instead of one shortcut concatenating sting in one line. As "safe" I mean that there is no standard in CSS in which order we should write rules inside shorthand. For example if we use padding and concatenate them without any additional checks:

// JS
container: {
  padding: {
    top: 10,
    left: 1,
    right: 20
  }
}

// Resulting CSS
.container {
  padding: 10px 1px 20px; /* Wrong! */
}

For avoiding such bug we should write additional logic for making shorthand rule if all values are correct, or write all rules separately it there is something wrong.

kof commented 3 years ago

That makes sense @typical000, thanks!