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

Use actual colors #1542

Closed
wants to merge 1 commit into from
Closed

Use actual colors #1542

wants to merge 1 commit into from

Conversation

glebm
Copy link

@glebm glebm commented Sep 3, 2015

Following up on the internal discussions. Words are cheap, code is gold.

  • Use actual colors everywhere, remove unquote stuff.
  • Define rgba-tpl function for generating (invalid) CSS templates (literal rgba($var, 0.5)).

@addyosmani
Copy link
Contributor

Thanks for the PR, @glebm. We're going to take some time to review the implementation here and discuss it with the core team. We appreciate you working on this.

@glebm
Copy link
Author

glebm commented Sep 3, 2015

Cheers! Reverted the strip-units change as unitless does not do what I thought it did.

* Use actual colors everywhere, remove `unquote` stuff.
* Since this project aims to generate invalid CSS for "easy" templating
  without Sass, we still need to generate literal `rgba($var, 0.5)`.
  Define `rgba-tpl` function for this purpose.
@gauntface
Copy link

This gets a big thumbs up from me.

@surma
Copy link
Contributor

surma commented Sep 3, 2015

👍

@Garbee
Copy link
Collaborator

Garbee commented Sep 3, 2015

This is a BC break for anyone using the Sass in their own build pipeline. Can't merge before 2.x.

@sgomes
Copy link
Contributor

sgomes commented Sep 3, 2015

I'm not sure how much of a BC break this is for folks using SASS, actually, @Garbee. If they're relying on the palette variables directly, perhaps (and I'd argue those should be considered private), but it otherwise seems compatible with our previous variable format.

It seems to me that if you're just defining $color-primary and friends, things should still work. I'll run some tests when I get a chance and comment again.

@glebm
Copy link
Author

glebm commented Sep 4, 2015

I have looked into making it backward-compatible:

  1. The rgba-tpl is already backwards-compatible, but could use a warning like this:

    @if str-index($color, ",") {
        // TODO: remove for 2.0 release.
        @warn "Please change your variables to use colors.
               Using strings is deprecated and will be removed in v2.0.";
    }
  2. There are a number of places that previously used unquote("rgb(...", such as:

       .mdl-color-text--deep-purple-A400 {
    -    color: unquote("rgb(#{$palette-deep-purple-A400})") !important;
    +    color: $palette-deep-purple-A400 !important;
       }

    These would require introducting another function such as color-or-string to be backwards-compatible. Let me know if you want this done, or rather postpone for v2.

@Garbee
Copy link
Collaborator

Garbee commented Sep 4, 2015

This has to be postponed to 2.0. Internally we can update all we want. Other developers however may have included MDL and used the Sass in their pipeline, then used the unquote hack in their code to get the colors. That means it is a breaking change.

@glebm
Copy link
Author

glebm commented Sep 4, 2015

No, this can be made backwards-compatible with the unquote hack. Nothing we can do if somebody is using unquote in their codebase, as e.g. rgba(#fff, 0.5) is invalid CSS, so has to go to v2.

@Garbee
Copy link
Collaborator

Garbee commented Sep 4, 2015

Ok, I'm confused on that one. If someone already has unquote("rgb(#{$palette-red-50})") defined in their Sass, wouldn't that break once RGB values are introduced? I don't see how it wouldn't unless we override unquote somehow.

@Garbee Garbee added this to the V2 milestone Nov 12, 2015
@Garbee Garbee self-assigned this Feb 18, 2016
sgomes pushed a commit that referenced this pull request Mar 2, 2016
This PR replaces our color variables to use actual colors rather
than strings. It also updates the template generation and
customizer to use '#name' style placeholders instead of '$name',
so that CSSO doesn't complain about the syntax.

This PR may later be invalidated if we decide to move away from SASS,
but for now it provides a good baseline for current development.
sgomes added a commit that referenced this pull request Mar 3, 2016
Use actual colors in SASS variables. Rework of #1542.
@sgomes
Copy link
Contributor

sgomes commented Mar 3, 2016

Merged with #4167, thanks @glebm and everyone who reviewed this!

@sgomes sgomes closed this Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants