use of !important on all generated styles

[montemishkin]

I see that "Consider removing !important from everything." is listed under TODO in the README.

Perhaps this could be discussed here?

[xymostech]

Sure! It hasn't been causing problems for us at KA, but we would still like to get rid of it. We have it around so that any of our random global CSS styles won't end up accidentally overriding aphrodite ones.

Have you been running into problems because of it?

[montemishkin]

Awesome! Yeah, I figured that it was kept in for something along those lines. I've run into issues with it in a few strange cases, so here's my best shot at distilling those into a minimal example...

[montemishkin]

One result of making everything !important is that styles passed to the style prop will not override styles given to className.

In most cases, one would not need to use both the className and style props on a single element. However, one situation where it might be useful to do so would be in a component that would have animated styles.

For example, we could imagine that MyComponent would be re-rendered with t changing over time. Ideally, the following code would animate the h1 font size to change over time when shouldAnimate is true, but stay at 10px when shouldAnimate is false:

function MyComponent({shouldAnimate, t}) {
    const style = shouldAnimate ? createStyle(t) : {}

    return (
        <h1 className={css(styles.header)} style={style}>
            Hello
        </h1>
    )
}

const styles = StyleSheet.create({
    header: {
        color: 'red',
        fontSize: '10px',
    },
})

function createStyle(t) {
    return {
        fontSize: `${t}px`,
    }
}

But since the font size is set to 10px (with !important) in styles.header, the font size will not actually be animated regardless of the value of shouldAnimate.

This could be solved by changing createStyle to instead read

function createStyle(t) {
    return {
        fontSize: `${t}px !important`,
    }
}

but this feels like more of a temporary fix than a solution since it requires the developer to keep in mind this particular case where specificity is not as they would expect.

Another work around is to create the StyleSheet based on t:

function MyComponent({shouldAnimate, t}) {
    const styles = createStyleSheet(shouldAnimate, t)

    return (
        <h1 className={css(styles.header)} style={style}>
            Hello
        </h1>
    )
}

function createStyleSheet(shouldAnimate, t) {
    return StyleSheet.create({
        header: {
            color: 'red',
            fontSize: shouldAnimate ? '10px' : `${t}px`,
        },
    })
}

but this suffers the overhead of running StyleSheet.create and css for all t, not to mention that the animation speed will then depend on the rate at which aphrodite chooses to inject the generated styles.

[montemishkin]

Anyways, I'm not certain where I stand on removing !important, but that's one example of where it hinders development.

[skitterm]

The !important command will be very handy with external CSS libraries that someone else controls the code for (like Bootstrap). It can be nigh impossible to override someone else's styles with just a classname when the library's author wrote something like .container .row or li .toggle > a -- I've found CSS Modules and React-look to be unworkable for this reason when trying to override third-party CSS.

[jossmac]

@xymostech seems like a good option here would be to make the use of !important an opt-in config setting for Aphrodite. That way projects with CSS files that may need to be overwritten can enable it, and developers who have concerns like those raised by @montemishkin can disable it.

Would you be open to this approach?

[xymostech]

@jossmac I would be open to that approach. I'm not sure what a good way to add such a setting would be. Maybe a second argument to StyleSheet.create? Like:

StyleSheet.create({
    ...
}, { important: false });

[montemishkin]

@xymostech That api seems reasonable to me. Then if somebody wanted to disable !important everywhere they could simply define

// createStyleSheet.js

import {StyleSheet} from 'aphrodite'

export default function createStyleSheet(obj) {
  return StyleSheet.create(obj, { important: false })
}

and then import createStyleSheet locally rather than StyleSheet from aphrodite.

[montemishkin]

I'd be happy to put together a PR adding this options argument to StyleSheet.create. Any objections?

[xymostech]

@montemishkin Sounds good to me!

[kentcdodds]

I like exposing the important API and I think it's a good call to make it opt-out. However, I think it'd great to eventually change it to opt-in (where the default is important: false). This way we only pay for the extra bytes when we need them (in a SSR scenario). But honestly, I should probably benchmark things because after gzipping I imagine that important is not really a big deal after compression... ¯_(ツ)_/¯

[kentcdodds]

Oh, also, hi @skitterm! 👋 Fancy meeting you here! (friends from school)

[kentcdodds]

Ok, so I did a quick test to see if gzipping made this not important on a size standpoint:

File	size (bytes)
original-important.html	62614
important.html.gz	11485
original-not-important.html	56378
not-important.html.gz	11316

The original HTML file comes from the JavaScript Air site. In the not-important files, I did a find/replace of all instances of !important and removed those. It looks like it does make a difference in size even when comparing gzipped files. So from a size perspective this should probably be addressed.

[sophiebits]

So !important is 10% before gzip and 1.5% after? That's really not that bad.

[kentcdodds]

Yeah, I guess I was thinking more in terms of size. But I just realized that what I thought was KB is actually bytes. So the difference in this case is a measly 0.1 KB so ¯_(ツ)_/¯

[ide]

Another reason to omit !important is that Google's AMP spec disallows it. This isn't a big deal since you probably could run css.content.replace('!important', '') 99.9% of the time though.

Edit: Also when browsers like Chrome display a little gripper in the bottom right corner of text areas to let the user resize them, the browsers resize the text areas by setting their width and height styles without !important. So if you style a text area with height: '3rem' using Aphrodite, Chrome's resizing doesn't override the !important style and the user can't resize the height.