Decompose shorthand/compound styles to their constituent parts

[mattborn]

I have some buttons in a nav with a default padding value:

var style = {
  padding: '10px 20px'
}

When selected, I add a thin left border + adjust only the left padding accordingly:

modifiers: [
  {
    selected: {
      borderLeft: '3px solid blue',
      paddingLeft: 17
    }
  }
]

Unfortunately, when the item becomes unselected, Radium is using 0 for the paddingLeft value instead of the original 20px value. I’m assuming this is because I don’t have paddingLeft as a property in the default styles:

I was able to fix this by changing paddingLeft to padding + using a more explicit string value:

modifiers: [
  {
    selected: {
      borderLeft: '3px solid blue',
      padding: '10px 20px 10px 17px'
    }
  }
]

My gut tells me Radium should be able to intelligently merge related properties like padding + paddingLeft, margin + marginTop, etc. on modifier changes.

1. Is this the expected behavior?
2. Is it recommended to be explicit with value changes like these?

[alexlande]

Interesting. Sounds like a bug to me, I'll check it out.

Unrelated to this bug, I would handle that style with box-shadow so that you don't need to deal with magic numbers for the padding: http://jsbin.com/gexavibeqa/2/edit?html,css,output

[mattborn]

@alexlande Thanks for the tip! Super helpful.

After wrestling with Radium all weekend + chatting with @balanceiskey this morning, I’m rethinking whether this is a problem or not. IE 9–11 still have problems with computing margin + padding when values aren’t explicit. Perhaps it’s better just to always be explicit with “shorthand” margin + padding and avoid …Top, …Right, …Bottom, …Left altogether. However, that might put this issue outside the scope of what Radium is trying to solve.

[azazdeaz]

Unfortunately this is the expected behavior by React: facebook/react#2231
They say the overlapping styles (like {padding: '2px', paddingLeft: '0px'}) are invalid.
So as i see, the best thing we can do is avoiding shorthands.

[alexlande]

Hmm, that's unfortunate. facebook/react#2013 has more good information on this as well.

[mattborn]

@azazdeaz @alexlande Thanks for tracking those down. I’m all for being more explicit, but I am still curious in exploring the parse + merge idea as @syranide mentions. I guess we’ll see.

[syranide]

@mattborn I've played around a little myself and from a technical perspective, by simply disallowing "complex" shorthands (like font and border) and decomposing "combined" shorthands (like borderColor and margin) you end up with a style object that can be merged reliably and doesn't need to be parsed in any way. Unintuitively, you don't even need to recombine them, the overhead seems almost identical for border vs all 4 border*.

PS. I even played around with a React branch that entirely drops the concept of classes for the user, it only takes style objects and creates/shares classes on-the-fly at runtime. It's really surprisingly perfomant as it turns out you really don't use very many unique style combinations at any point in time. You avoid all the issues traditionally associated with classes (even specificity).

[mattborn]

@syranide Which React branch drops classes? Sounds nuts + awesome at the same time.

[syranide]

@mattborn https://github.com/syranide/react/tree/classstyle , className is actually still supported but only for testing purposes, specificity is an issue. It's only an experiment so the code is crap, but it seems to me that it is viable and more practical than having to pre-allocate classes, perf shouldn't be a significant issue. So all you do is write inline styles for all elements which are then allocated/cached/shared transparently at run-time. (PS. not an official React branch, but it could certainly be made to plugin into React at some point).

[ianobermiller]

@alexlande Nice tip on box-shadow!

I think for Radium, we should decompose shorthand/compound styles to their constituent parts before passing it on to React. That gives us another value add above plain inline styles and also eliminates this weird edge case.

[alexlande]

👍

[mattborn]

👍

[ianobermiller]

Something like this might be useful: https://github.com/ksanders/grunt-css-longhand/blob/master/tasks/css_longhand.js

[ianobermiller]

Alternatively, we could just throw a warning whenever you have a longhand and a shorthand property in the same style object, e.g. padding and padding-left. We should really have a DEV mode for this sort of thing. MDN has more information on shorthand properties. The list is:

• background
• font
• margin
• border
• border-top
• border-right
• border-bottom
• border-left
• border-width
• border-color
• border-style
• transition
• transform
• padding
• list-style
• border-radius

[mattborn]

Thanks for taking care of this @ianobermiller!

[natew]

@ianobermiller I think the right way to handle this would be to allow mixing, but to take the more specific as an override. For example:

$section = {
    border: '1px solid rgba(0,0,0,0.1)',
    borderLeft: 'none',
    borderRight: 'none'
}

This is a really common pattern that lets you not have to specify the border multiple times. And it's pretty obvious you'd want the more specific value to override in every case.

Thoughts?

[kumarharsh]

@natew yup, I agree to your point here.

[ianobermiller]

Yes, that is the ideal way to do it. Unfortunately to allow that style would require Radium to parse the border rule into all its constituent parts. This is difficult for all but the most trivial shorthands, margin and padding, for example. I've seen some libraries that would like to do this (https://github.com/ActionIQ/style-builder), but I haven't yet found a complete one that accepts all the messed up junk a browser does and is still reasonably compact.

As an example, 1px solid black is the correct way to write the border shorthand, but solid 1px black works just as well.

[kumarharsh]

@ianobermiller it's not actually browser junk, and 1px solid black is not necessarily the correct way. The spec just defines that the first length unit should be construed as the thickness of the border, the first type enum should be the type of the border, and the first color should be the color. This is useful to figure out the omitted values:

From the spec:

Name: border-top, border-right, border-bottom, border-left
Value: <line-width>; || <line-style> || <color>

This is a shorthand property for setting the width, style, and color of the top, right, bottom, and left border of a box. Omitted values are set to their initial values.

[kumarharsh]

This style of the spec is even more visible in rules such as animation, wherein the first time value is ALWAYS the duration, and the second time value is ALWAYS the delay, and so on...

[ianobermiller]

Good point, thanks @kumarharsh. The point still stands that I haven't yet found a JS library to properly parse shorthand properties :)

[ianobermiller]

Ok, I admit I did not look very well: https://www.npmjs.com/package/css-shorthand-expand. It certainly isn't lightweight, though.

[natew]

@ianobermiller Any update on this? I upgraded Radium on a project but now am getting heaps of warnings. I suppose I could add in css-shorthand on my own for the meantime.

In all honesty, I'd trade the modifiers and all nesting for being able to use the mixed modes. Modifiers have almost no use to me, but mixed mode styles I use in nearly every component.

[natew]

Just looked at css-shorthand-expand, doesn't seem that big, no? Looks like ~100 lines total including all packages.

[natew]

And by 200 I mean maybe 500 but within an order of magnitude :)

[ianobermiller]

Including all dependencies, css-shorthand-expand is roughly 5k (after uglify + gzip). This would probably be better as a plugin, given the size.

[alexlande]

Another relatively slim module that someone pointed me to for this purpose: https://github.com/ActionIQ/style-builder

Agree that this behavior makes the most sense as a plugin.

[alexlande]

And a blog post about it: http://www.actioniq.co/blog/reactjs-cramping-my-style/

Thanks @rgerstenberger

[ianobermiller]

@alexlande I looked at style-builder too, but it only supports border, margin, and padding.

[williamboman]

Cross-posting comment from #218 (comment).

Quite annoying to see these warnings when it's done deliberately, e.g.;

// utils.radium.js
export const resetBoxModel = { margin: 0, padding: 0, border: 0 };

// foo.jsx
import {resetBoxModel} from './utils.radium';

const styles = {
    base: {
        ...resetBoxModel,
        marginRight: 5,
        paddingLeft: 10,
    }
};

or are there better ways of doing this?

[ianobermiller]

@williamboman The warnings exist exactly for this reason. Your code is susceptible to the React issue mentioned above: facebook/react#2231

Considering that margin, padding, and border are the most common culprits here, perhaps we pull in style-builder just for those so the majority of people don't see the warning.

[ghost]

@ianobermiller added in support for other ordering of border properties (like solid 1px black) to style builder. Should now support any order of properties as well as providing only one or two (v1.0.6).

[williamboman]

@williamboman The warnings exist exactly for this reason. Your code is susceptible to the React issue mentioned above: facebook/react#2231

Ah, thanks for the heads up, missed that one.

Considering that margin, padding, and border are the most common culprits here, perhaps we pull in style-builder just for those so the majority of people don't see the warning.

Maybe make style-builder an optional dependency and only use it if it exists. I'm more inclined to work around my issue by expanding the shorthand rather than fixing it during run-time through an additional dependency.

[ianobermiller]

It should also be possible for us to include style-builder only in the DEV
build, which might be good enough.

On Thu, Nov 19, 2015 at 8:46 AM William Boman notifications@github.com
wrote:

@williamboman https://github.com/williamboman The warnings exist
exactly for this reason. Your code is susceptible to the React issue
mentioned above: facebook/react#2231
facebook/react#2231

Ah, thanks for the heads up, missed that one.

Considering that margin, padding, and border are the most common culprits
here, perhaps we pull in style-builder just for those so the majority of
people don't see the warning.

Maybe make style-builder an optional dependency and only use it if it
exists. I'm more inclined to work around my issue by expanding the
shorthand rather than fixing it during run-time through an additional
dependency.

—
Reply to this email directly or view it on GitHub
#95 (comment)
.

[gaearon]

You are welcome to contribute your experiences to this discussion: facebook/react#6348.

[sethyuan]

It seems React 15+ has this problem solved? If so, is there a way to turn off the warnings from Radium? It's kind of annoying.

[gaearon]

I don't think React 15 did anything to solve this.

[downsider]

I've reached this thread after searching for a solution to a different problem, but the two seems to overlap and what I want to be able to do would possibly solve both issues.

I've been frustrated that while I can use variables for specific values in my styles, when it comes to more complex margin, padding and border definitions, I have to resort to writing a CSS value into a string. This feels out of place given that the rest of the definition are pure Javascript.

What I would like to do is something like this:

const big = 20;
const small = 10;

const styles = {
  demo: {
    margin: [small, small, big, big], 
    // margin: '10px 10px 20px 20px

    padding: [big, small],
    // padding: '20px 10px'
  }
}

This example has the margin and padding shorthand values set as arrays, which can then be compiled into a CSS string.
However, this system would also open up the opportunity to override specific values before the CSS was compiled, like so:

const styles = {
  demo: {
    margin: [0, 10],
    marginLeft: 30
  }
}

In this case, we would decompose margin into values for top, right, bottom and left, override the value for marginLeft and either re-compose the string for margin or use the marginTop, etc... properties directly. As we're using javascript to pre-process the CSS attributes, we shouldn't fall foul of the overlapping styles bug

In order to extend the model to variable shorthand properties like border or background, we would need to be explicit about which part of the definition were which:

const styles = {
  demo: {
    border: {
      width: 1,
      style: 'solid',
      color: 'red'
    },
    borderBottomColor: 'green'
  }
}

Again, border would be decomposed into values for the four sides, which could then be overridden by subsequent property definitions and recomposed into CSS. This syntax isn't quite as succinct as the CSS version, but it seems like it would solve the problem.

Does this sounds like a viable solution? It looks like it would work to me, but I don't know the innards of Radium and the consequences this change would have.

[ianobermiller]

I like the idea! This should be very easy to do with a plugin. You can see a recent PR for an example: #703.