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

Add basic atomising CSS #13549

Merged
merged 39 commits into from Jul 13, 2016

Conversation

Projects
None yet
5 participants
@sndrs
Copy link
Contributor

commented Jul 8, 2016

What does this change?

Adds an atomisable set of sass files, as a preliminary step to enabling it generally.

This is mainly so that @NataliaLKB and I can test it and try and iron out the annoying and show-stopping oversights that aren't considered in this PR.

Known to-dos:

  1. docs
  2. update dom in dev

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

it enables us to get going on a real-world test of the principle. If the principle works, it should mean isolated CSS modules with the smallest possible CSS output.

Does this affect other platforms - Amp, Apps, etc?

no

Screenshots

.new-header__nav {
    @include fs-textSans(6);
    display: flex;
    align-items: center;
    width: 100%;

    @include mq($until: mobileLandscape) {
        font-size: 5vw;
        padding-left: ($gs-gutter / 2);
        padding-right: ($gs-gutter / 2);
        // Position after pseudo element which forces line break
        order: 1;
        flex: 1;
        justify-content: space-between;
    }
}
<nav class="@CssClass("new-header__nav")">

turns into

screen shot 2016-07-08 at 16 42 28

screen shot 2016-07-08 at 16 42 56

## Request for comment

@johnduffell @NataliaLKB

@johnduffell

This comment has been minimized.

Copy link
Member

commented Jul 8, 2016

fantastic! I am really excited by this, new ways of thinking could really be a game changer here. code looks good, although I assume the actual atomiser code is loaded in automatically? 👍 can't wait to see the results.

@NataliaLKB

This comment has been minimized.

Copy link
Member

commented Jul 8, 2016

lgtm 👍 .

This is really exciting! I am looking forward to testing this 😄

lazy val lookup: Map[String, List[String]] = Get(cssMap(mapResource))

def apply(path: String): String = {
if (useCssMap) {

This comment has been minimized.

Copy link
@rich-nguyen

rich-nguyen Jul 11, 2016

Contributor

I would inline useHashedBundles here, at the location it's being used, for easier grep-ing. It makes it easier to understand where the system behaviour diverges for dev-asset loading. I doubt anyone will ever pass a second param to CssMap, and ... (see comment below)

This comment has been minimized.

Copy link
@johnduffell

johnduffell Jul 11, 2016

Member

Good point, I agree that special parameters only used for testing is not ideal - and I hadn't thought about greppability. What I was going for was making the classes as pure as possible so that the tests don't need to know anything about the internals (composition over inheritance).
An overridable method for fencing off the behaviour not needed in the tests is a good way in such a simple class, although arguably adds more complexity than a defaulted parameter.

Assets/AssetsTest also use the same pattern so I'll look at that too - https://github.com/guardian/frontend/blob/master/common/test/assets/AssetsTest.scala#L9

This comment has been minimized.

Copy link
@sndrs

sndrs Jul 12, 2016

Author Contributor

i've updated the way it works to always output the atomic classes and the original one, so that special param is gone now anyway

@@ -13,4 +13,11 @@ class AssetsTest extends FlatSpec with Matchers with OneAppPerSuite {
static("zen3").toString should be("simon says" + "the quieter you become the more you are able to hear.")
}

"CssMap" should "collect css maps" in {
val static = new CssMap("assets/testclass-map.json", true)

This comment has been minimized.

Copy link
@rich-nguyen

rich-nguyen Jul 11, 2016

Contributor

... using true here forces people to build their assets a certain way just to pass tests. You should be able to test the CssMap in this situation by extending the class, and overriding the bits you need to stub.

@regiskuckaertz

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2016

Nice! We have a big incentive to make this work for ads, as most blockers hide DOM elements based on class/id values.

Let me know if I can test this on some of our own components.

@sndrs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2016

Let me know if I can test this on some of our own components.

yeah soon i hope. this will have inevitable issues (alpha really) and i want to iron them out before recommending trying it (beta). you can definitely prepare for it by only using single class selectors though

@@ -0,0 +1,4 @@
.New-header {

This comment has been minimized.

Copy link
@rich-nguyen

rich-nguyen Jul 12, 2016

Contributor

are you using caps for atomised classes?

This comment has been minimized.

Copy link
@sndrs

sndrs Jul 12, 2016

Author Contributor

maybe – it really needs to be used for real, but i'd imagined a more suit-like capitalised name for the module. we'll need to see how it writes (hence not officially opening this up)

@rich-nguyen

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2016

👍 agree with @regiskuckaertz , this would be interesting if we can think about ad containers

@regiskuckaertz

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2016

Single class selectors will be a challenge, but definitely worth a try 😅

@sndrs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2016

Single class selectors will be a challenge, but definitely worth a try

my suspicion is that it can be achieved, it's (hopefully) just moving logic from selectors to templates

@regiskuckaertz

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2016

Yes, I reckon it is. Speaking of atomic classes, wdyt of having (1) a typographic scale and (2) a naming convention of classes scoped to particular breakpoints?

Merge branch 'master' of github.com:guardian/frontend into atomic-css
# Conflicts:
#	common/app/assets/assets.scala

@sndrs sndrs merged commit c9cfe4c into master Jul 13, 2016

1 check passed

continuous-integration/teamcity Finished TeamCity Build dotcom :: pull requests : Tests passed: 1880, ignored: 14
Details

@sndrs sndrs deleted the atomic-css branch Jul 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.