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

Initial work to add createGlobalStyle functionality #1416

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@JamieDixon
Contributor

JamieDixon commented Jan 9, 2018

Implements #1333

This is very much a WIP and open for discussion

@JamieDixon JamieDixon added the WIP label Jan 9, 2018

@mxstbr-bot

This comment has been minimized.

mxstbr-bot commented Jan 9, 2018

Warnings
⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@mxstbr

This comment has been minimized.

Member

mxstbr commented Jan 9, 2018

Awesome!

In terms of removing styles, we need to add a new method to StyleSheet.js which removes tags based on component IDs. That should call tag.removeComponent(id), which we'd need to implement to essentially reverse the work done in tag.addComponent(id). Should be doable maybe?

@JamieDixon

This comment has been minimized.

Contributor

JamieDixon commented Jan 9, 2018

I'll be looking into removeComponent today.

@kitten

This comment has been minimized.

Member

kitten commented Jan 9, 2018

@mxstbr in a future (!) PR we should probably add interpolation support as well 😄

Edit: This could also use the BaseStyledComponent class (we could extract it) to save some space and to get the prop/interpolation/theme support immediately: https://github.com/styled-components/styled-components/blob/master/src/models/StyledComponent.js#L55

@kitten

Some minor problems that we'll need to fix first 😉 Thanks for picking this up! 🙌

return class extends React.Component {
componentId = ''
componentDidMount() {

This comment has been minimized.

@kitten

kitten Jan 9, 2018

Member

Here the component should use componentWillMount instead 😉 otherwise this wouldn't work correctly for SSR. It's also important as we'll want the global styles to apply immediately before all DOM nodes mount (that might be referred to in or affected by these global styles).

This comment has been minimized.

@JamieDixon

JamieDixon Jan 9, 2018

Contributor

Thanks! Adjusted in df87dd7

const rules = css(strings, ...interpolations)
const hash = hashStr(JSON.stringify(rules))
this.componentId = `sc-global-${hash}`
if (StyleSheet.instance.hasInjectedComponent(this.componentId)) return

This comment has been minimized.

@kitten

This comment has been minimized.

@JamieDixon

JamieDixon Jan 9, 2018

Contributor

Sweet. Thanks for the tips. Adjusted in c6155ec

@mxstbr

This comment has been minimized.

Member

mxstbr commented Jan 9, 2018

Damnnn df87dd7 🔥 🔥 🔥

Do you reckon we'll be able to use that for all styled components in the future? That'd be extremely rad.

@kitten

This comment has been minimized.

Member

kitten commented Jan 9, 2018

@mxstbr I don't think we should remove any rules for styled components as that'd be unnecessary work. We don't care whether rules remain around or not since we control their classnames 😁

@JamieDixon

This comment has been minimized.

Contributor

JamieDixon commented Jan 9, 2018

Is there a printWidth prop missing from .prettierrc? I think my Atom is mucking up your nice formatting.

@JamieDixon JamieDixon force-pushed the JamieDixon:create-global-style branch 2 times, most recently from 34bb1f4 to 40ef36e Jan 9, 2018

@kitten

This comment has been minimized.

Member

kitten commented Jan 9, 2018

@JamieDixon that's definitely possible (I haven't had a look) but normally the default should be used. Maybe the Atom plugin falls back to its setting instead? That'd be horrible 😆 Feel free to add it to the .prettierrc if you don't mind

Edit: Also, can you remove the Ran yarn format commit please? The formatting should only be done by lint-staged and just pushing another commit that fixes formatting will also then show your commit as the last edit... not the end of the world of course, just a nit 😉

@JamieDixon JamieDixon force-pushed the JamieDixon:create-global-style branch from 40ef36e to 769e3ba Jan 9, 2018

@JamieDixon

This comment has been minimized.

Contributor

JamieDixon commented Jan 9, 2018

Thanks @philpl. I've dropped the Ran yarn format commit and added a new one updating the .prettierrc file. 👍

@JamieDixon JamieDixon force-pushed the JamieDixon:create-global-style branch 2 times, most recently from 08935a6 to 5f40aa9 Jan 11, 2018

@JamieDixon

This comment has been minimized.

Contributor

JamieDixon commented Jan 15, 2018

Is there more you'd like me to do on this one guys? I took a quick look into BaseStyledComponent but couldn't get it working. I'm happy to take another look if it's stopping this PR being approved?

@mxstbr

Looks good to me code wise, can we undo the changelog.md reformatting? That breaks git blame, which can be vital sometimes.

I'd also like some unit tests, but I can look at that if you don't want to write those 😉 We'll also need to add documentation to the website and deprecate injectGlobal for the next major.

@JamieDixon JamieDixon force-pushed the JamieDixon:create-global-style branch from 5f40aa9 to 8bff338 Jan 15, 2018

@JamieDixon JamieDixon force-pushed the JamieDixon:create-global-style branch from 8bff338 to 0985a8a Jan 15, 2018

@JamieDixon

This comment has been minimized.

Contributor

JamieDixon commented Jan 15, 2018

@mxstbr I've dropped the CHANGELOG commit and sent a new one with just the change we want. I need to figure out why prettier changed all the things.

@mxstbr

This comment has been minimized.

Member

mxstbr commented Jan 15, 2018

Hold on, why is that changelog entry under 2.4.1 and not under unreleased? Pretty sure that won't be going out in 2.4.1?

@JamieDixon

This comment has been minimized.

Contributor

JamieDixon commented Jan 15, 2018

Good point. Lemme fix that up.

@JamieDixon JamieDixon force-pushed the JamieDixon:create-global-style branch from 63bca43 to a79cc26 Jan 15, 2018

@JamieDixon

This comment has been minimized.

Contributor

JamieDixon commented Jan 15, 2018

Fixed

@marionebl marionebl referenced this pull request Feb 5, 2018

Closed

Add component-based global styling API #1493

5 of 5 tasks complete
@kitten

This comment has been minimized.

Member

kitten commented Feb 11, 2018

@JamieDixon work was picked up in #1493; Please take a look to make sure that it's still on track 🙌

@JamieDixon

This comment has been minimized.

Contributor

JamieDixon commented Feb 16, 2018

@kitten Cool. I hope what I did here was still useful 😬

@kitten

This comment has been minimized.

Member

kitten commented Feb 16, 2018

@JamieDixon Definitely! Thanks for kicking this work off 👍

@jarodtaylor

This comment has been minimized.

jarodtaylor commented Feb 24, 2018

This is sexy.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Jun 2, 2018

Closing in favor of #1493 since it's a continuation and evolution of this one 😸

@probablyup probablyup closed this Jun 2, 2018

@imbhargav5 imbhargav5 referenced this pull request Jul 26, 2018

Merged

Add component-based global styling API #1867

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment