-
Notifications
You must be signed in to change notification settings - Fork 187
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
Conversation
Another option for naming would be to have |
Another option is to export a version of css that doesn't use Then if could be imported
or, if that's confusing (since
I like this because |
@zgotsch Woah I did not know that @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. |
Looking forward to this change... I like being able to tweak CSS in the browser and |
@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 ( I should have a chance to get to the README bit later this week. |
Ok, sorry for the delay. I added a section on how to opt out of Proofreads welcome, but otherwise I think this is ready for merge :) |
Looks like there's a merge conflict. Rebase? |
Ok I rebased to Khan:master, fixed the merge conflict, then force pushed to my branch. Two questions:
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! |
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 |
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 |
Hm yea I guess we could auto-wrap style attributes with Aphrodite |
Would love to use this soon. |
@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 It strikes me that for a given project, you're likely to either want Aphrodite to always use |
Why not have it be a function on the |
maybe |
Oh, I like that @kevinbarabash 👍 |
Noob question, why bother? Why isn't it like normal CSS where it's up to the author to specify which rules have important? |
See this comment: #25 (comment) |
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
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
This is something @jossmac and I are closely following, until we can remove 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 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 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. |
I'm not certain that the default case shouldn't be If we did go with a breaking change them I imagine a codemod could be written really easily to help people upgrade without trouble... |
Just realised that 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
... 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
... but it doesn't work. Because the 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 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. |
How I've solved this problem is merging my But I can totally see how doing that extra work would be frustrating and it'd be easier to be able to simply 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 :-) |
@jlfwong Yes, once we decide on the API for this I think it is ready to merge. I agree that For my personal use cases, having Regardless, it seems to me that we have the following decisions to make:
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! |
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:
It will use -webkit-box-pack rule. I'm not sure now, but it could end up loosing performance? |
@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. |
@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 I don't think a deprecation strategy involving logging messages when you use the default "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 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 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- @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 The codemod would convert:
to
and
to
|
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. |
@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
New component
The resulting color of 'Hello' is red, since 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 Soo... I think I'm going to backpeddle here and say the default should not be without My bikeshed contribution syntax is then to use |
Sorry to keep shedding on this but I was really excited to read @jlfwong's initial response above, before @spicyj flipped the decision around...
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 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.
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 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 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 😃 |
@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 The benefit of the 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 If we decide later that it should be default, we can deprecate |
I guess this would work great for everyone. It is also a more pragmatic solution, since it doesn't incur any breaking changes. |
A few questions I'd like to bring up before the implementation gets too far are:
|
Just a comment on:
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.
If you needed someone's stuff to not have |
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 This makes sense to me; the developer using the styles gets to decide whether the resulting styles get |
@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 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
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, 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 |
@JedWatson I'm confused. For a reusable component like react-select, what's the downside of using |
Sorry, finally responding to this thread. I think that there are some avenues that you're not exploring here @JedWatson :-)
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 |
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. |
@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 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 More importantly though, having @kentcdodds your example answers how I could implement the equivalent functionality of the I don't see how calling 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 The more we argue about this, the more I think that using css-modules hasn't had to resort to applying
|
I thought that I'd answered that with my last statement:
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 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:
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 |
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.
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.
👍 I don't think I see "aphrodite, generate me the stylesheet and add 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? |
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 |
@montemishkin Did you still want to own this PR and change it to use the |
👏 yay! I'm glad cooler heads have prevailed - this has been blocking my use of aphrodite in a project. Mergey merge merge? |
The ability to suppress 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! |
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. |
Not yet ready for merge.
Looking for feedback :)
In #25, @xymostech mentioned adding an api like
which sounded good to me.
However, the actual CSS is not generated at the call to
StyleSheet.create
, but rather at the call tocss
. Thus I thought it made more sense for the!important
option to be provided at the call tocss
. (Open to other ideas on this though.)Since
css
is variadic, I exported another functionunimportantCss
(open to better name) which does exactly whatcss
does, but doesn't add!important
to the CSS it generates.