Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Add custom props postcss plugin #69

Merged
merged 1 commit into from
Mar 26, 2018
Merged

Conversation

siimonevans
Copy link
Member

@siimonevans siimonevans commented Feb 25, 2018

Polyfill for custom properties on older browsers (in our case IE11). Tied to build:prod.

Input:

:root {
    --color: hotpink;
}

body {
    background-color: var(--color);
}

Output:

:root {
    --color: hotpink;
}

body {
    background-color: hotpink;
    background-color: var(--color);
}

Simples.

@siimonevans siimonevans self-assigned this Feb 25, 2018
Copy link
Member

@helenb helenb left a comment

Choose a reason for hiding this comment

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

Hiya, this looks ok to me on the face of it. If I've understood correctly, this plugin simply adds fallbacks for browsers that don't support custom properties? Which is why we only need to run it in prod not dev? Also, I haven't tested on a local build (can do if you think it would be beneficial), but have you double checked it works OK running straight after autoprefixer? I couldn't think what problems there might be off the top of my head but I guess it's as well to test to make sure.

@siimonevans
Copy link
Member Author

@helenb thanks for checking this out :)

Hiya, this looks ok to me on the face of it. If I've understood correctly, this plugin simply adds fallbacks for browsers that don't support custom properties? Which is why we only need to run it in prod not dev?

Correct.

Also, I haven't tested on a local build (can do if you think it would be beneficial)

I'd appreciate local testing in case node version/local config doesn't play nicely.

have you double checked it works OK running straight after autoprefixer?

Yeah. I think running it last is the safest, although running it before autoprefixer shouldn't change anything.

@helenb
Copy link
Member

helenb commented Feb 26, 2018

Sure will run a local test, probably tomorrow.

Copy link
Member

@nicklee nicklee left a comment

Choose a reason for hiding this comment

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

Looks great

@nicklee
Copy link
Member

nicklee commented Feb 27, 2018

We should add an ignore to the sass lint SpaceAroundOperator rule, otherwise it complains every time you use a custom property. Unfortunately I can't find the property to disable in the documentation, looks like it might be a WIP:

sasstools/sass-lint#877

@helenb
Copy link
Member

helenb commented Feb 27, 2018

Ran a test locally and it worked fine.

Re the space-around-operator issue the docs for that are here: https://github.com/sasstools/sass-lint/blob/master/docs/rules/space-around-operator.md. Could be worth updating sass-lint to see if that has been solved, as the issue @nicklee linked to is about a different linting error for custom properties.

@siimonevans siimonevans merged commit 250f80c into master Mar 26, 2018
@siimonevans siimonevans deleted the custom-properties-fallback branch March 26, 2018 11:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants