-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Component-based API for global styling #1333
Comments
I think this would be a valuable change. Myself and @geelen originally decided against going down this route because it seemed more tedious to use, but as you said it has a bunch of nice properties. A couple more are:
I'm open to doing this. My only concern is that I don't like overloading the /* First possibility: injectGlobal now just returns a component */
const GlobalStyles = injectGlobal`
body {
background: blue;
}
`
/* Second possiblity: Different API */
const GlobalStyles = injectGlobalAsComponent`
body {
background: blue;
}
`
/* Third possibility: Radium-like <Style /> component */
<Style>{`
body {
background: blue;
}
`}</Style> I'd personally prefer the first way. I'd rather not we have multiple ways of doing things, and we could break this in a major version without troubles. |
@mxstbr I'm totally down for replacing I do think though that the name I don't think this necessarily depends on #1324, but we do need to implement a way to remove styles from our sheet (or alternatively append & replace classes to |
Doesn't depend on #1324 at all, but enables more functionality for the Yeah you're right, on second thought With appending and replacing that doesn't matter for the first version of this component (though we should really tackle that) since right now people just work around the dynamicism with a function that gets called when the styles should change I assume. So they're injecting a bunch of times anyway. |
createGlobalStyle sounds good!
…On Wed, Nov 29, 2017 at 3:06 PM Max Stoiber ***@***.***> wrote:
Doesn't depend on #1324
<#1324> at
all, but enables more functionality for the StyleSheetManager!
Yeah you're right, on second thought injectGlobal returning a component
is kind of weird. What about const Style = createGlobalStyle or something
to that effect?
With appending and replacing that doesn't matter for the first version of
this component (though we should really tackle that) since right now people
just work around the dynamicism with a function that gets called when the
styles should change I assume. So they're injecting a bunch of times anyway.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1333 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADWlmrhB90b9BmcIj_R8LBKc0jwd_H5ks5s7WTWgaJpZM4Qu7YR>
.
|
This would be a huge benefit. Unifying the theming of components with global styles like font-face and page |
Alright, so it seems that everybody likes e.g. import {createGlobalStyle} from 'styled-components'
const Style = createGlobalStyle`
${p => p.noScroll && `body { overflow: hidden; }`};
` as an API? What are the next steps to make this happen? Is this a deep change to how the CSS is managed? |
I don't think so, the only thing I'm not 100% sure about is re: server-side rendering and rehydration—would definitely be worth kicking off a first PR so that @geelen can take a look at that though! |
Continue my thoughts from #1354 Helper Proposed API examples:
|
Something I've realized: The issue with introducing a new API like Definitely not ideal... 😕 |
Hmm, we can have both and deprecate injectGlobal?
…On Sat, Dec 16, 2017, 11:56 AM Max Stoiber, ***@***.***> wrote:
Something I've realized: The issue with introducing a new API like
injectGlobalStyle is that it breaks all our tooling since that relies on
the injectGlobal name. (think Babel plugin, stylelint processor,...)
Definitely not ideal... 😕
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1333 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADWljjo8ZlByFa5NU12TyiZHVjVyZnYks5tA6HDgaJpZM4Qu7YR>
.
|
Yes of course, just a thing to note that it'll be a bit more work after we implement this API. |
Alright, let's get started on this—anybody want to tackle a first PR that implements this new API? We already have tests for most of the global behavior, so it should just be a matter of rewriting those tests to use the new API (at which point they'll all fail) and then slowly rebuild the new API until all tests pass again! |
Just to clarify: is the blessed API the following? import {createGlobalStyle} from 'styled-components'
const Style = createGlobalStyle`
${p => p.noScroll && `body { overflow: hidden; }`};
` |
Yep! See #1416, would love if you could pick that up and implement some tests so we can ship it 😊 |
I'll give this a shot, might get stuck on some WIP state though. |
This is a thing in v4 :) |
Hmm. Error |
Well you can continue to use injectGlobal in v3. However v4 has a few good features and perf upgrades so in case you want to switch you may have to move to createGlobalStyle. |
@imbhargav5 after latest v3 update it just forces me with severity " I want to say some kind of deprecations should provide a way to switch on the new feature and to remove that deprecation warning. But currently the deprecation warning doesn't provide any alternatives to do that without upgrading to v4. |
Yeah... I think I’m just going to take it out. After more thought it doesn’t make sense to put in a non-actionable warning. |
Via #730 (comment)
It would be great to have a React component for global styles that does the actual injecting (rendering null), removing the CSS when it is unmounted.
So instead of
you'd do
The only differences with regular styled components would be:
This way:
The text was updated successfully, but these errors were encountered: