Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make "!important" optional #41

Closed
wants to merge 2 commits into from
Closed

Make "!important" optional #41

wants to merge 2 commits into from

Conversation

montemishkin
Copy link
Contributor

Not yet ready for merge.

Looking for feedback :)

In #25, @xymostech mentioned adding an api like

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

which sounded good to me.

However, the actual CSS is not generated at the call to StyleSheet.create, but rather at the call to css. Thus I thought it made more sense for the !important option to be provided at the call to css. (Open to other ideas on this though.)

Since css is variadic, I exported another function unimportantCss (open to better name) which does exactly what css does, but doesn't add !important to the CSS it generates.

@montemishkin
Copy link
Contributor Author

Another option for naming would be to have css(...styleDefinitions) and css.unimportant(...styleDefinitions).

@zgotsch
Copy link
Contributor

zgotsch commented Apr 25, 2016

Another option is to export a version of css that doesn't use !important.

Then if could be imported

import {unimportantCss as css} from 'aphrodite';

or, if that's confusing (since css might not be what someone expects when looking at the file)

import {unimportantCss as ucss} from 'aphrodite';

I like this because css.unimportant is a lot of characters and css calls often appear on already long lines.

@xymostech
Copy link
Contributor

@zgotsch Woah I did not know that as syntax even existed.

@montemishkin I like this approach! Sorry I missed this and didn't respond! It looks like the change is pretty trivial so I'm okay with this. You said this isn't ready for merge but I don't see any problems! If you could add some docs about this to the README that would be great.

@kevinbarabash
Copy link
Member

Looking forward to this change... I like being able to tweak CSS in the browser and !important makes it difficult.

@montemishkin
Copy link
Contributor Author

@zgotsch Sorry if I am misinterpreting, but I think my commit already does exactly what you are suggesting? Thanks for the input on which name you prefer though :)

@xymostech Thanks! No problem :) Ok, yeah I said that it wasn't ready mostly because of the lack of mention in the README, and because I thought I would see what everybody thought about the naming (unimportantCss vs. css.unimportant vs. something else). Any feelings on this?

I should have a chance to get to the README bit later this week.

@montemishkin
Copy link
Contributor Author

Ok, sorry for the delay. I added a section on how to opt out of !important to the README.

Proofreads welcome, but otherwise I think this is ready for merge :)

@kentcdodds
Copy link
Contributor

Looks like there's a merge conflict. Rebase?

@montemishkin
Copy link
Contributor Author

Ok I rebased to Khan:master, fixed the merge conflict, then force pushed to my branch.

Two questions:

  1. Is that satisfactory?
  2. Was that the "best" practice? (if you have the time to explain)

I don't have a huge amount of experience contributing to others' projects and just want to make sure I'm making that process as easy as possible for both them and me!

@natew
Copy link
Contributor

natew commented May 4, 2016

Just wanted to chime in and say 👍 with a valid use case.

It's actually impossible to override styles using inline styles in React now (at least in 0.15, see facebook/react#1881). React doesn't allow !important for inline styles which means you can't override Aphrodite-set styles. For us, this means some tricky workarounds for allowing dynamic inline styles to work.

@kentcdodds
Copy link
Contributor

kentcdodds commented May 4, 2016

allowing dynamic inline styles to work.

What's the use-case for dynamic inline styles? Can't you just do everything with aphrodite? If not, I think the real issue is supporting those use cases.

That said, I'm still in favor of exposing an API to skip !important :-)

@natew
Copy link
Contributor

natew commented May 5, 2016

Hm yea I guess we could auto-wrap style attributes with Aphrodite css. That should work. I wasn't sure if performance was an issue there but I'll give it a run.

@salzhrani
Copy link

Would love to use this soon.

@jlfwong
Copy link
Collaborator

jlfwong commented Jun 15, 2016

@montemishkin Sorry for the long lag time here.

Is this written as intended for merge now? If so, can you please update the PR description to reflect this, along with including an example usage? I see that you've updated the README, which is great, but I like to make it easy to review PR's after the fact.

I'm not super sold on the name unimportantCss, but perhaps others like it? Would love others' feedback on the name here (bikeshedding mode: ENGAGE).

It strikes me that for a given project, you're likely to either want Aphrodite to always use !important or never, but maybe I'm not following peoples' use case for this.

@kentcdodds
Copy link
Contributor

Why not have it be a function on the css export like: css.unimportant or something....

@kevinbarabash
Copy link
Member

maybe import { css } from 'aphrodite/unimportant'

@kentcdodds
Copy link
Contributor

Oh, I like that @kevinbarabash 👍

@salzhrani
Copy link

Noob question, why bother? Why isn't it like normal CSS where it's up to the author to specify which rules have important?

@kentcdodds
Copy link
Contributor

See this comment: #25 (comment)

jlfwong added a commit that referenced this pull request Jun 16, 2016
Imagine you're trying to introduce Aphrodite gradually into an existing codebase
that already has extensive test coverage for rendering components.

If those tests are run in node without any fake DOM being constructed, your
previously working tests will now fail with the following error:

     Error: Cannot automatically buffer without a document

The introduction of StyleSheetTestUtils makes it easy to suppress such problems
by preventing Aphrodite from trying to interact with the DOM at all.

The new API would be usable in your tests like so (using Mocha syntax for
demonstration purposes):

    import { StyleSheetTestUtils } from 'aphrodite';

    beforeEach(() => {
        StyleSheetTestUtils.suppressStyleInjection();
    });

    afterEach(() => {
        StyleSheetTestUtils.clearBufferAndResumeStyleInjection();
    });

Meant to solve the problem presented in #41
jlfwong added a commit that referenced this pull request Jun 16, 2016
Imagine you're trying to introduce Aphrodite gradually into an existing codebase
that already has extensive test coverage for rendering components.

If those tests are run in node without any fake DOM being constructed, your
previously working tests will now fail with the following error:

     Error: Cannot automatically buffer without a document

The introduction of StyleSheetTestUtils makes it easy to suppress such problems
by preventing Aphrodite from trying to interact with the DOM at all.

The new API would be usable in your tests like so (using Mocha syntax for
demonstration purposes):

    import { StyleSheetTestUtils } from 'aphrodite';

    beforeEach(() => {
        StyleSheetTestUtils.suppressStyleInjection();
    });

    afterEach(() => {
        StyleSheetTestUtils.clearBufferAndResumeStyleInjection();
    });

I intentionally did not expose an API returning CSS content here to allow us
flexibility to change the CSS formatting or class name generation without
considering it a breaking API change.

Meant to solve the problem presented in #41
@JedWatson
Copy link

JedWatson commented Jun 17, 2016

This is something @jossmac and I are closely following, until we can remove !important from aphrodite we're spamming inline styles in our codebase with !important 😞

re the bikeshedding question, I'd love a breaking change so it becomes the following:

// default "works like you'd expect" mode, no !important addition to styles
import { css } from 'aphrodite';
// explicit opt-in to adding !important on styles
import { important as css } from 'aphrodite';

Fundamentally for new users, I strongly believe adding !important is the opposite of what you want as the default case, not the least because then using Aphrodite breaks expected behaviour of overriding styles applied in the stylesheet with styles applied inline.

I appreciate this may be a hard sell... so if there's no appetite for this now how about we go with a future-proof transition strategy:

// new unimportant variant
import { unimportantCSS as css } from 'aphrodite';
// new important variant
import { importantCSS as css } from 'aphrodite';
// backwards-compatibility logs a warning in development
import { css } from 'aphrodite';

Push that as a minor bump, start warning anyone who's using the css method that behaviour will change, users can start being explicit about the decision in their codebases, and in the next major switch the default can change and the warning goes away.

This is pretty similar to how the change from owner to parent based context behaviour was handled between React 0.12, 0.13 and 0.14.

@kentcdodds
Copy link
Contributor

I'm not certain that the default case shouldn't be !important. I think that there's a good reason to leave that as the default. Because the whole idea is to style specific components and not worry about the cascading part of CSS. This may be an"ugly" implementation detail, but that's all it is.

If we did go with a breaking change them I imagine a codemod could be written really easily to help people upgrade without trouble...

@JedWatson
Copy link

JedWatson commented Jun 17, 2016

Just realised that 0.4.0 was released 10 hours ago, so in my comment above replace "minor" with "patch" and "major" with "minor" 😉

@kentcdodds

Imagine there is a Button component. It has no margin style by default. In one particular place on your site, you want it to have marginTop: 10. You do this:

<Button style={{ marginTop: 10 }} onClick={this.handleClick}>My Great Button</Button>

... and everything works as expected.

Now imagine there is an Input component. It has a 5px top margin by default. In one particular place on your site, you want it to have marginTop: 20, so you do this:

<InputField style={{ marginTop: 20 }} onChange={this.handleChange} value={this.state.inputValue} />

... but it doesn't work. Because the marginTop style applied by the Aphrodite Stylesheet in the InputField component has put !important in the stylesheet (whether or not the component author intended that style to be important, because this behaviour is the default).

This is confusing and frustrating. You need to know the implementation detail of the component (which styles are specified in the aphrodite stylesheet), and the behaviour of aphrodite, and understand the Important spec in order to use inline styles.

If you're in an environment where other stylesheets can mess with your component styles, then for sure - use the importantCss variant. That's a win. Being able to opt out of worrying about the cascading part of CSS is a great feature, and something that Aphrodite can make really easy (as opposed to, say css modules where you need to mark up every. single. line.)

It's not an "ugly" implementation detail, IMO, as much as a great feature that's specifically not how CSS works by default anywhere. Being opt-in makes it explicit and predictable.

@kentcdodds
Copy link
Contributor

How I've solved this problem is merging my StyleSheet.create({/* this object */}) with a prop that's provided to me by the user of the component.

But I can totally see how doing that extra work would be frustrating and it'd be easier to be able to simply style={props.style}. But I'm still not convinced that making unimportant the default is necessary. Because you'd have to explicitly expose the inline style API to users of your component to allow them to use inline styles as you're demonstrating anyway. If you're going to go to that trouble, then you can import the correct version of css that doesn't attach the !important.

Honestly though, I think I'm good with either. I've bikeshedded enough on this :-) Looking forward to hearing from people at KA to make a decision and then someone can work on the implementation :-)

@montemishkin
Copy link
Contributor Author

montemishkin commented Jun 17, 2016

@jlfwong Yes, once we decide on the API for this I think it is ready to merge.

I agree that unimportantCss might not be the best name, and I had mentioned the css.unimportant idea at the start of the discussion. At this point however, I think both @JedWatson and @kentcdodds thoughts (on whether !important should be default or opt-in) have merit.

For my personal use cases, having css be unimportant by default would be great. I also lean towards agreeing with @JedWatson that unimportant by default is closer to what I would expect as a new user, and easier to reason about as an existing aphrodite user. At the same time, I completely understand that others might want to avoid such a direction for aphrodite.

Regardless, it seems to me that we have the following decisions to make:

  • Should we aspire to eventually make css unimportant by default?
    • If so, do we want to introduce the breaking change immediately, or incrementally?
    • If not, what do we want to name unimportantCss?

Anyways, once we can come to some sort of consensus on how to move forward with this, I am happy to fix up the PR description, etc. Yay! Thanks everybody for being a part of the discussion!

@iamrane
Copy link

iamrane commented Jun 22, 2016

One thing I noticed with the !important flag is that when having it, at least in chrome, the browser always uses the prefixed rule.

For example:

 justify-content: center !important;

-webkit-justify-content: center !important;

-ms-flex-pack: center !important;

-webkit-box-pack: center !important;

It will use -webkit-box-pack rule. I'm not sure now, but it could end up loosing performance?

@nettofarah
Copy link

@montemishkin Any update on this?

I'm more than happy to take over this PR or help any way I can if you don't have the bandwidth to work on this right now.

@jlfwong
Copy link
Collaborator

jlfwong commented Jul 5, 2016

@nettofarah Thanks for offering to take it over! I think @montemishkin is waiting on @xymostech and/or me to make a call here.

I agree in principle that css() should probably not be !important by default. It was a design decision early on to meet Khan Academy's needs and probably doesn't make a sensible default for a community project.

I don't think a deprecation strategy involving logging messages when you use the default css() call is going to work, because it'll be spammy. And what would it say?

"If you'd like to continue using the default !important appending, use 'aphrodite/with-bang-important' instead, otherwise keep doing what you're doing, but the behaviour will switch all of a sudden when you upgrade?"

I'm not super keen on conceptually having a css, a cssWithImportant and a cssWithoutImportant, where css is initially identical to cssWithImportant and later becomes equivalent to cssWithoutImportant. Because if people want the non-important behaviour, they have to codemod to switch to cssWithoutImportant, then switch back to the original css once they do the major version upgrade. Seems like more hassle than just doing a codemod when you do the major version bump.

IMO, it's okay to be a little more aggressive here and just make the breaking change (in a major version bump, of course), providing access to the old behaviour via aphrodite/with-bang-important, along with codemod for ES5 and ES6 style imports. Hopefully people are aggressive enough about using semver that this won't hurt anyone.

Khan Academy, I believe, would only be able to safely upgrade to 1.0.0 then using the codemod, then could possibly use the non-!important behaviour moving forward.

@xymostech am I understanding the impact at Khan Academy properly here? Are you comfortable with the proposed change?

To be clear, here is what I'm suggesting:

In a major version change to 1.0.0, the existing css function no longer appends !important, and a new aphrodite/with-bang-important is introduced for back compat, along with a codemod.

The codemod would convert:

import { StyleSheet, css } from 'aphrodite';

to

import { StyleSheet, css } from 'aphrodite/with-bang-important'

and

var/const { StyleSheet, css} = require('aphrodite');

to

var/const { StyleSheet, css} = require('aphrodite/with-bang-important');

@sophiebits
Copy link

Why would most people not want !important? It seems like it is a good idea generally and prevents specificity problems when interoperating with legacy stylesheets, and has only a couple specific downsides mentioned here. It is also much easier to determine when !important is a problem then to determine when the lack of it might be, which also points to !important as a better default.

@jlfwong
Copy link
Collaborator

jlfwong commented Jul 5, 2016

@spicyj Ah... I thought I was misunderstanding the interop problems, then I realized my initial intuition for interop problems being there was likely right.

Here's an example I'm worried about due to interop, just as @spicyj was probably alluding to:

Legacy stylesheet

.foo div {
    color: red;
}

New component

const Component = () => <div className='foo'>
    <div className={css(styles.bar)}>
        Hello
    </div>
</div>

const styles = StyleSheet.create({
    bar: {
        color: 'green'
    }
});

The resulting color of 'Hello' is red, since .foo div has a higher specificity. This behaviour isn't terribly surprising here, but it would be if a was instead some class name set on an element outside of the component (like on the body).

Regarding surprising behaviour of inline styles not overriding styles set in the stylesheet, the intended usage patterns of Aphrodite were to remove the need for using style={} altogether, though I understand that does require libraries that use Aphrodite as a dependency to force their consumers to use Aphrodite too.

Soo... I think I'm going to backpeddle here and say the default should not be without !important, and we should just make it easy to opt into not using !important.

My bikeshed contribution syntax is then to use aphrodite/without-bang-important as the import name for people that would like to avoid using !important, and not do any major version bump.

@JedWatson
Copy link

JedWatson commented Jul 5, 2016

Sorry to keep shedding on this but I was really excited to read @jlfwong's initial response above, before @spicyj flipped the decision around...

Why would most people not want !important?

Simply, it breaks the style prop. As a web developer, you think you know how class names and inline styles work, yet they don't work that way with Aphrodite by default. There are other reasons (see @ide's comment on #25) - the fact is, adding !important is a hack that neither browsers nor users expect by default (it breaking input field resize is a great example of an unexpected side-effect).

And we don't know what other side effects may be introduced with other features and browser updates in the future. I assign risk to it not being the way things are usually done in css. Important is a workaround at best and should be used judiciously. Please let's not make it the default for a whole generation of web developers!

I'd suggest that the only valid reason to turn it on is in an environment where you have legacy stylesheets in play that you want aphrodite styles to trump.

It is also much easier to determine when !important is a problem then to determine when the lack of it might be, which also points to !important as a better default.

I have watched people at work getting frustrated with Aphrodite over this, and I choose to disagree. It's pretty obvious in the web inspector where styles are inherited from; adding !important to classes when you need it (or making the whole switch if you find yourself doing that often) is better imo than adding important to inline styles when you need to.

In our work we've used Aphrodite on a number of new React projects and none of them have existing styles we need to override. For me, this is skating to where the puck will be. Over the next year or two, will more developers pick up React and Aphrodite for new projects? Or gradually replace pieces of existing projects? I'm not sure.

My argument is that it's better to say - "if you are adding Aphrodite to a site with legacy stylesheets, use the important variant" rather than "if you want to have the style prop work as expected and / or don't understand edge cases related to !important, use the unimportant variant."

Bah! Thanks for listening and sorry for continuing to fight. However you go thanks for your work on Aphrodite, it's awesome and I don't usually argue this much on GitHub issues 😃

@jlfwong
Copy link
Collaborator

jlfwong commented Jul 5, 2016

@JedWatson Certainly no need to apologize for continuing to fight! The only resource being sacrificed here is your time, since you're contributing very usefully to this debate :)

Aphrodite was not designed with the intention for it to be over-rideable with the style prop because it was meant to replace the need for using the style prop. Perhaps this could be more clearly communicated in the docs.

The benefit of the !important default is also one of reasoning about your code. With the generated !important, it is really unlikely that someone might accidentally be over-riding it with a stylesheet somewhere. This kind of reasoning benefit is akin to reasoning about things with const or immutability: you don't need to understand your whole codebase to know what the component will do in isolation.

It's clear to me, however, that there is a real desire for this feature to exist, regardless of whether or not it is default. I'm afraid the discussion of default-ness here is blocking people from using a feature they want.

So I'd prefer to move forward with a minor version change that introduces the aphrodite/without-bang-important import.

If we decide later that it should be default, we can deprecate aphrodite/without-bang-important. I'm not super excited about that deprecation path, but I don't want to stall this feature for the debate.

@nettofarah
Copy link

So I'd prefer to move forward with a minor version change that introduces the aphrodite/without-bang-important import.

I guess this would work great for everyone. It is also a more pragmatic solution, since it doesn't incur any breaking changes.

@ide
Copy link
Contributor

ide commented Jul 5, 2016

A few questions I'd like to bring up before the implementation gets too far are:

  1. Are there options other than toggling !important that may come along later? If so, then using an options object scales better than defining a new module for each possible combination of options. Ex: css([s1, s2], { important: false }).
  2. Should toggling !important be an option on css or StyleSheet.create?
  3. Does it make sense to be able to toggle !important globally? Is it useful for the caller of StyleSheet.render to globally specify whether to use !important by default? The idea is you could npm install someone else's component that uses Aphrodite and you'd control whether !important shows up.

@kentcdodds
Copy link
Contributor

Just a comment on:

Does it make sense to be able to toggle !important globally?

Definitely want to avoid doing that. Global config puts the burden on the developer to have to understand the entire system to know what the result of the code they're looking at will be. Better to make it explicit.

The idea is you could npm install someone else's component that uses Aphrodite and you'd control whether !important shows up.

If you needed someone's stuff to not have !important then you file an issue and ask them to expose an API to disable !important. Or even better, have them expose an API for you to pass your own styles.

@xymostech
Copy link
Contributor

Is it useful for the caller of StyleSheet.render to globally specify whether to use !important by default? The idea is you could npm install someone else's component that uses Aphrodite and you'd control whether !important shows up.

That's actually an interesting question. The way I imagined it (probably because it's the way that makes the most sense in the code right now), the aphrodite/without-bang-important would only affect the css() function. That is, when generating styles with StyleSheet.create, where that StyleSheet came from doesn't matter. Only which version of css you imported from matters, so import { css } from 'aphrodite' will always produce !important styles, and import { css } from 'aphrodite/without-bang-important will always produce no-!important ones.

This makes sense to me; the developer using the styles gets to decide whether the resulting styles get !important-ed. But maybe that API would be confusing? Do other people have thoughts?

@JedWatson
Copy link

@kentcdodds and I can't seem to agree on anything these days 😛

Why wouldn't the burden be on the developer (i.e. someone using a component written with Aphrodite) to opt-in to !important? The use-case is that you're using Aphrodite in a site with conflicting, legacy stylesheets, which is in no way an assumption I want to make as a component author.

React Select is a good example. It currently ships with both SCSS and LESS stylesheets that users can implement their own build process for (so they can control variables), as well as a compiled CSS file (which they can always override with their own styles) in case they don't want either SASS or LESS in their project.

... it's a mess. I've learned a lot maintaining that project.

I'd love to implement a JavaScript-based style system, and Aphrodite and JSS are my two favourites (haven't picked a "winner" yet...)

The requirements are

  • being able to theme the component, including using it multiple times in a site/app with different themes
  • inline style support (override styles in render without changing the theme)
  • server rendering support
  • plug and play (ideally, you just npm install react-select and start using it in your app)

However, I absolutely don't want to make a decision about whether the styles should be !important.

If someone's dropping RS into a project where styles conflict (there have been issues on sites w/ Bootstrap, for example) then great, !important all the way. But you lose the runtime style prop without adding !important to your inline styles as well. I'm OK with that.

In other projects - especially greenfield ones - no way, I don't want to enforce that behaviour.

It's bugging me that if I go with Aphrodite, there's literally no way I can avoid making that decision on behalf of my users. This project gets 180k downloads per month. There are currently 80 PRs and 380 Issues. I'm swamped. Every decision I make, especially if it's enforced, increases the maintenance burden because everybody wants something different.

Sorry for the rant, I just want to explain where I'm coming from. I think letting whoever is actually calling Stylesheet.render choose whether to include !important as an option is a fantastic idea 😀

@sophiebits
Copy link

sophiebits commented Jul 8, 2016

@JedWatson I'm confused. For a reusable component like react-select, what's the downside of using !important? Is it that you expect people to override certain styles using a regular legacy .css file and class name selectors?

@kentcdodds
Copy link
Contributor

Sorry, finally responding to this thread. I think that there are some avenues that you're not exploring here @JedWatson :-)

It's bugging me that if I go with Aphrodite, there's literally no way I can avoid making that decision on behalf of my users.

This is literally not at all true.

So first of all, if you're exposing a JavaScript API to your component for people to consume, why can't you also expose an API for people to override styles? You'd have to do this same thing if you allow inline styles anyway. It'd be trivial to do this:

export default function MyComponent({stuff, styleOverrides}) {
  const styles = getStyles(styleOverrides)
  return <div className={css(styles.component)}>{stuff}</div>
}

function getStyles(styleOverrides) {
  return StyleSheet.create(merge({ // http://npm.im/lodash.merge
    component: {
      color: 'red',
      backgroundColor: 'green',
    }
  }, styleOverrides))
}

Also, if we implement a mechanism to not include !important on the css, then you as a library author could just use that mechanism if you want people to be able to override the styles.

@ide
Copy link
Contributor

ide commented Jul 13, 2016

if we implement a mechanism to not include !important on the css, then you as a library author could just use that mechanism if you want people to be able to override the styles.

This makes sense but I think it's worth thinking about the cost to expose that mechanism. If every library author needs to expose style overrides in order for the caller to disable !important (ex: for AMP which disallows !important), that requires more energy and coordination than requiring no work on behalf of each library author if the caller of StyleSheet.render can opt into a blanket override.

@JedWatson
Copy link

@ide this raises an interesting point... I wonder what the best way of exposing an API for a component like react-select would be in terms of calling Stylesheet.render.

If the user (i.e. developer installing the component into their app) uses Aphrodite, it makes sense for their app to call it. If they use css-modules, to "just work" the component would need to call it.

@spicyj that's actually a valid use case, there are a bunch of users who do that (or take the .css from the dist build and just modify it directly). Not how I'd do things personally, but each to their own :)

More importantly though, having !important on styles (a) breaks the inline style prop unless the component user adds !important to those as well, which is unexpected and ugly, and (b) behaves unexpectedly or breaks (e.g. AMP)

@kentcdodds your example answers how I could implement the equivalent functionality of the style prop through Aphrodite by creating styles at render-time, not how I'd let users opt in or out of the !important hack.

I don't see how calling StyleSheet.create in a render method is a good idea. Surely that's not going to perform (?) - this concern is already a pain when it comes to theming (i.e. the equivalent of variables in a LESS stylesheet), you can solve that by caching stylesheets based on theme config, opening it up to runtime styles is a whole other level of complexity. My take on your suggestion is it seems trivial in isolation but isn't when you take into account all the other concerns.

Consider this, instead:

export default function MyComponent({stuff, style}) {
  return <div className={css(styles.component)} style={style}>{stuff}</div>
}

const styles = StyleSheet.create({
    component: {
      color: 'red',
      backgroundColor: 'green',
    }
  }
}

It's much simpler, and considerably more scalable.

To me it's a warning sign that we're talking about how to restore the style prop's functionality through Aphrodite, which is only a workaround you need if !important is applied to all the generated styles in the first place. We're patching over a hack with another hack that hurts performance.

The more we argue about this, the more I think that using !important to fix the "what if other styles have higher precedence" problem is the wrong way to approach it. There are other ways of aggressively raising precedence, including IDs - maybe there's a better solution in that? (see #97)

css-modules hasn't had to resort to applying !important universally and it's a widely praised solution to the same problem Aphrodite is tackling*, maybe @markdalgleish can weigh in with their experience or another perspective..?

* I'm talking about the first line in the readme, "Support for colocating your styles with your JavaScript component"

@kentcdodds
Copy link
Contributor

not how I'd let users opt in or out of the !important hack.

I thought that I'd answered that with my last statement:

Also, if we implement a mechanism to not include !important on the css, then you as a library author could just use that mechanism if you want people to be able to override the styles.

I didn't think I needed to provide an example of that as well, but it could be done easily:

import {StyleSheet, css, unimportantCSS} from 'aphrodite' // however it's implemented
import componentConfig from './my-config-that-users-can-set-globally' // could be done globally for your lib or on a component-by-component basis...

let cssToUse = css
if (componentConfig.overrideStyles) {
  cssToUse = unimportantCSS
}

export default function MyComponent({stuff, style}) {
  return <div className={cssToUse(styles.component)} style={style}>{stuff}</div>
}

const styles = StyleSheet.create({
    component: {
      color: 'red',
      backgroundColor: 'green',
    }
  }
}

Of course, I expect you would consider this to place too great a burden on the end user (to force them to configure things #jsfatigue). So you could just use the unimportantCSS by default and have it configurable the other way around...

I also want to say from a design patterns perspective it's a bad idea to configure things globally. This is the same reason you can't configure React itself globally:

Global configuration doesn’t work well with composition so we don’t provide it.

If you could configure React globally, then you could look at React code, but not be sure intuitively of what's going to happen. I don't want this to happen to Aphrodite.


I think the real issue here is some of us are coming from an application perspective and others are coming from a library perspective. Each has different use cases. I think that we should just implement unimportantCSS and let library authors use that if they want to allow for overrides with the style attribute. I think that'll cover things well.

@JedWatson
Copy link

So you could just use the unimportantCSS by default and have it configurable the other way around...

That's the way I'd personally configure it, I'm more concerned about consistency between components from different authors (some will support it, others won't, everyone will come up with their own way of doing things). The question is can we solve this at Aphrodite's design level? I don't know. I think it's worth really trying.

Global configuration doesn’t work well with composition so we don’t provide it.

I get where you're coming from.

I guess the question is who should make the decision (component author vs. user, which holds up across teams in a large company as much as in open source) and then what's the best API we can design.

If you could configure React globally, then you could look at React code, but not be sure intuitively of what's going to happen. I don't want this to happen to Aphrodite.

👍

I don't think I see "aphrodite, generate me the stylesheet and add !important to everything" to be config in the same way you do. It makes sense as part of Aphrodite's API. I see the addition of !important as an additional transform on the output, not part of Aphrodite's behaviour.

Think about it in terms of the autoprefixer maybe. That's not optional, but it's not hard to imagine it could be. If it was, should each component decide whether to apply it? Or would that config be part of the call to generate the stylesheet that the user is in control of?

@kentcdodds
Copy link
Contributor

Yeah, I'm done bikeshedding. I obviously don't care as much as @JedWatson does. Actually, I feel like I've been convinced. The ability to not clash with global styles seems like a feature you could opt-into. So I say let's make the default be exclude !important and you can opt-into adding !important.

@jlfwong
Copy link
Collaborator

jlfwong commented Jul 16, 2016

@montemishkin Did you still want to own this PR and change it to use the aphrodite/without-bang-important option? I'm happy to implement this myself, but don't want to deprive you of contribution credit if you were adamant about pursuing it.

@josephg
Copy link

josephg commented Jul 19, 2016

👏 yay! I'm glad cooler heads have prevailed - this has been blocking my use of aphrodite in a project.

Mergey merge merge?

@jlfwong
Copy link
Collaborator

jlfwong commented Jul 20, 2016

The ability to suppress !important appending is now available via aphrodite/no-important, as explained in the README.md: https://github.com/Khan/aphrodite#disabling-important. The feature is available as of aphrodite@0.5.0, which I just published. This was fixed in #104.

Thanks to @montemishkin for writing an initial concept for this, and for sparking more of the discussion. Thanks to everyone in this thread for contributing your thoughts!

@jlfwong jlfwong closed this Jul 20, 2016
@montemishkin
Copy link
Contributor Author

Hey all, glad to see a consensus was reached! Sorry I wasn't able to get back earlier, I'm on a long bike trip with no computer.
Thanks to everyone who contributed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet