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

Ui: add a way to write combinator selectors #17761

Closed
wants to merge 9 commits into from
Closed

Conversation

sndrs
Copy link
Member

@sndrs sndrs commented Sep 5, 2017

What does this change?

adds styled-jsx to UI

What is the value of this and can you measure success?

gives us a way to write more complex CSS than styletron allows:

<div>
    <Logo style={logo} />
    <style jsx>{`
        div :global(.guardian) {
            fill: ${colour.brandBlueDark};
        }
    `}</style>
</div>

i wanted to abstract styled-jsx away more, so we could have something like:

const __css = `
    div :global(.guardian) {
        fill: ${colour.brandBlueDark};
    }
`;
<Logo style={logo} __styleExpensively__={{ __css }} />

but styled-jsx does't allow you to use props, only strings you compose or import explicitly (yet). if this seems useful, when that is released we change the way this works.

what this means is that we have both styletron and styled-jsx available. styletron should be the default but this is here when you just cannot get what's needed.

Examples

instead of the atomic styles that styletron gives us, you end up with the something like the following (given the example above):

<style id="__jsx-style-4143013755">
    div[data-jsx="4143013755"] .guardian{fill:#00456e}
</style>

<div data-jsx="4143013755">
    <svg width="320" height="60" viewBox="0 0 320 60">
        <path class="guardian" />
    </svg>
</div>

and although they claim it's only 2kb gzipped, adding styled-jsx has bumped the client bundle from 13kb to 23kb (gzipped).

most of that is core-js though, which suggests we have a problem with the babel set up more that anything else. will look into that separately.

is this a good idea?

not sure how i feel about this, having the both. welcome thoughts on this...

@PRBuilds
Copy link

PRBuilds commented Sep 5, 2017

PRbuilds results:

Screenshots
desktop.pngtablet.pngmobile.pngwide.png

Exceptions (0)
thrown-exceptions.js

A11y validation (3)
a11y-report.txt

Apache Benchmark Load Testing
loadtesting.txt

--automated message

@@ -56,7 +56,7 @@ trait JavascriptRendering extends Logging {
private def prescript: ByteArrayInputStream = {
val pre =
"""
|var global = global || this, self = self || this, window = window || this;
|var global = global || this, self = self || this, window = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when you just remove the window variable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that's a more sensible idea!

@@ -74,6 +74,10 @@ const config = {
'node_modules', // default location, but we're overiding above, so it needs to be explicit
],
extensions: ['.js', '.jsx'],
alias: {
// some libs expect react, this stops them bundling it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@NataliaLKB NataliaLKB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really interesting and good solution to still have the styles scoped! And great PR description.

I still don't know how to motivate people not to use this way of styling things. Maybe we could have a linting rule which warns people to only use this when they have to?
We could try and make it so that linting rule only runs on new code so its not too noisy

@GHaberis
Copy link
Contributor

GHaberis commented Sep 6, 2017

Thanks for this @sndrs, very useful. It's good seeing how they work together.

I do worry that splitting styles across a styletron import and styled-jsx import, each with differing syntax (scss rules vs styles as strings) and differing ways of applying the rules in the component would lead to a confusing developer experience. There'd be an expectation that developers would need to understand why there are 2 options and when best to apply them. I agree with @NataliaLKB that we'd want to encourage styletron over styled-jsx where styletron is sufficient in order to get the benefits of atomisation etc.

@sndrs
Copy link
Member Author

sndrs commented Sep 6, 2017

@GHaberis I agree, but making it clear and harder to use the expensive stuff would be handy i think. once styled-jsx allows you to pass CSS from props it should be easier to create a more normalised api, perhaps something like:

// force you to import this
import ExpensivelyStyled from 'utils/ExpensivelyStyled';

// add new *.expensive.scss extension (visible in your file browser)
// imports as string rather than JS object, so extension is necessary
import pretendTheseStylesAreOk from './styles.expensive.scss';

// force you to keep writing the word 'expensive'
<ExpensivelyStyled expensiveStyles={pretendTheseStylesAreOk}>
    <Logo style={logo} />
</ExpensivelyStyled>

@TBonnin
Copy link
Contributor

TBonnin commented Sep 6, 2017

<div>
    <Logo style={logo} />
    <style jsx>{`
        div :global(.guardian) {
            fill: ${colour.brandBlueDark};
        }
    `}</style>
</div>

That looks quite strange to me. It's treated as a string right?
Does it mean no autocompletion, code coloring, etc..?

@sndrs
Copy link
Member Author

sndrs commented Sep 6, 2017

@TBonnin yeah it's a little odd, that's just how styled-jsx works. that css could be an import though, from a CSS file.

one thing that bugs me about it is that div :global(.guardian) means the style is scoped to the parent <div>, which is perfect, but it's not obvious from looking at the code, especially the CSS is moved to an external file

@sndrs
Copy link
Member Author

sndrs commented Sep 6, 2017

i'm going to close this – it's close but it's not good enough. looking at emotion as an alternative.

@sndrs sndrs closed this Sep 6, 2017
@sndrs sndrs mentioned this pull request Sep 13, 2017
@sndrs sndrs deleted the ui/styled-jsx branch October 11, 2017 11:17
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.

5 participants