Add extra color methods #66

Closed
wants to merge 21 commits into
from

Conversation

Projects
None yet
3 participants
@eunomie
Contributor

eunomie commented Oct 26, 2015

  • saturateColor(color, amount)
  • desaturateColor(color, amount)
  • greyscale(color)
  • lighten(color, amount)
  • darken(color, amount)
  • spin(color, hue_angle)

*Color are a bit redundant but saturate is a recognized css functions in CssFunctionNode.

Those color manipulation methods are compatible with Less expect spin because Less convert to RGB and I only use HSL manipulations.

Test case

GSS file to be compiled in test_gss_colors.css

@def BASE_COLOR #5a7bd8;
.div1 {
 background: BASE_COLOR;
}
.div2 {
 background: lighten(BASE_COLOR, 10);
}
.div3 {
 background: darken(BASE_COLOR, 10);
}
.div4 {
 background: saturateColor(BASE_COLOR, 20);
}
.div5 {
 background: desaturateColor(BASE_COLOR, 20);
}
.div6 {
 background: greyscale(BASE_COLOR);
}
.div7 {
 background: spin(BASE_COLOR, 30);
}
.div8 {
 background: spin(BASE_COLOR, -30);
}
<html>
       <head>
               <title>Gss test</title>
               <style>
                       div {
                               width: 400px;
                               height: 100px;
                               text-align: center;
                               font-family: arial;
                       }
               </style>
               <link href="test_gss_colors.css" rel="stylesheet" />
       </head>
       <body>
               <div class="div1"><pre>
@def BASE_COLOR #5a7bd8;
{ background: BASE_COLOR; }</pre></div>
               <div class="div2"><pre>
{ background: lighten(BASE_COLOR, 10); }
{ background: #839ce2; }</pre></div>
               <div class="div3"><pre>
{ background: darken(BASE_COLOR, 10); }
{ background: #315ace; }</pre></div>
               <div class="div4"><pre>
{ background: saturateColor(BASE_COLOR, 20); }
{ background: #4671ec; }</pre></div>
               <div class="div5"><pre>
{ background: desaturateColor(BASE_COLOR, 20); }
{ background: #6e85c4; }</pre></div>
               <div class="div6"><pre>
{ background: greyscale(BASE_COLOR); }
{ background: #999999; }</pre></div>
               <div class="div7"><pre>
{ background: spin(BASE_COLOR, 30); }
{ background: #785ad8; }</pre></div>
               <div class="div8"><pre>
{ background: spin(BASE_COLOR, -30); }
{ background: #5abad8; }</pre></div>
       </body>
</html>

See attached file for result.
prilvwhrhbg7b5ptya030legyu5x3ofwx2z2aasspgk

@googlebot googlebot added the cla: yes label Oct 26, 2015

@eunomie eunomie referenced this pull request Oct 26, 2015

Open

Extra color functions #28

@iflan

This comment has been minimized.

Show comment
Hide comment
@iflan

iflan Oct 27, 2015

Member

Awesome! Thank you. I'm going through your change now.

Member

iflan commented Oct 27, 2015

Awesome! Thank you. I'm going through your change now.

@@ -370,7 +375,7 @@ public String getCallResultString(List<String> args)
baseColorString, args.get(1), args.get(2), args.get(3));
}
- protected String addHsbToCssColor(
+ public static String addHsbToCssColor(

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

Why the change to public static? The idea with protected is to indicate that it's not part of the public interface of the class for callers, but can be used by subclasses. I know that there is the parseBoolean method above, but I don't think it should be public static either. What do you think?

@iflan

iflan Oct 27, 2015

Member

Why the change to public static? The idea with protected is to indicate that it's not part of the public interface of the class for callers, but can be used by subclasses. I know that there is the parseBoolean method above, but I don't think it should be public static either. What do you think?

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

Ahh, I see that you're re-using this method down on line 503. Hmm. Yes, I would agree that you wouldn't want to have an instance of AddHsbToCssColor to color for that. I'm going to have to think a bit on this one.

@iflan

iflan Oct 27, 2015

Member

Ahh, I see that you're re-using this method down on line 503. Hmm. Yes, I would agree that you wouldn't want to have an instance of AddHsbToCssColor to color for that. I'm going to have to think a bit on this one.

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

I was just looking at BaseBlendColors. I wonder if you can't do something similar here by making all of the functions that use AddHsbToCssColor.addHsbToCssColor inherit from the same base class, including what is today AddHsbToCssColor. What do you think?

@iflan

iflan Oct 27, 2015

Member

I was just looking at BaseBlendColors. I wonder if you can't do something similar here by making all of the functions that use AddHsbToCssColor.addHsbToCssColor inherit from the same base class, including what is today AddHsbToCssColor. What do you think?

This comment has been minimized.

@eunomie

eunomie Oct 27, 2015

Contributor

I'm working on it. I'm adding some methods to manipulate colors using HSL color space (which make more easy some operations).
Yes I can extract some functions and inherit from a base class, it'll be better.

@eunomie

eunomie Oct 27, 2015

Contributor

I'm working on it. I'm adding some methods to manipulate colors using HSL color space (which make more easy some operations).
Yes I can extract some functions and inherit from a base class, it'll be better.

@@ -404,7 +409,7 @@ protected String addHsbToCssColor(
* @param brightnessToAdd The amount of brightness to add (can be negative)
* @return A CSS String representing the new color
*/
- public String addHsbToCssColor(String baseColorString,
+ public static String addHsbToCssColor(String baseColorString,

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

Same as above.

@iflan

iflan Oct 27, 2015

Member

Same as above.

@@ -433,7 +438,7 @@ public String addHsbToCssColor(String baseColorString,
* @param brightnessToAdd The amount of brightness to add
* @return The modified color
*/
- public Color addValuesToHsbComponents(Color baseColor,
+ public static Color addValuesToHsbComponents(Color baseColor,

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

And again. :-)

@iflan

iflan Oct 27, 2015

Member

And again. :-)

@@ -459,6 +464,298 @@ public Color addValuesToHsbComponents(Color baseColor,
}
+ /**
+ * Saturate the specified color. First argument is the color, second is the
+ * amount of saturation (from 0 to 100)

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

Is that 0-100% more than the current saturation? Or will 0 essentially make it grey? I think you should be explicit in the docs, if you can be.

@iflan

iflan Oct 27, 2015

Member

Is that 0-100% more than the current saturation? Or will 0 essentially make it grey? I think you should be explicit in the docs, if you can be.

+ if (arg2 instanceof CssNumericNode) {
+ numeric2 = (CssNumericNode)arg2;
+ } else {
+ String message = "Arguments number 2 must be CssNumericNodes";

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

How about:

String message = "Argument 2 must be a CssNumericNode";
@iflan

iflan Oct 27, 2015

Member

How about:

String message = "Argument 2 must be a CssNumericNode";
+ if (arg2 instanceof CssNumericNode) {
+ numeric2 = (CssNumericNode) arg2;
+ } else {
+ String message = "Arguments number 2 must be CssNumericNodes";

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

Like above.

@iflan

iflan Oct 27, 2015

Member

Like above.

+ public void testSaturateColor_otherLocale() throws GssFunctionException {
+ Locale.setDefault(Locale.FRANCE);
+ try {
+

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

Extra line.

@iflan

iflan Oct 27, 2015

Member

Extra line.

+
+ GssFunctions.SaturateColor function = new GssFunctions.SaturateColor();
+ assertEquals("#2F5BD8",
+ function.getCallResultString(ImmutableList.of("#5a7bd8", "20")));

This comment has been minimized.

@iflan

iflan Oct 27, 2015

Member

The _otherLocale tests only matter if there is a fractional part to the result. See issue #14.

@iflan

iflan Oct 27, 2015

Member

The _otherLocale tests only matter if there is a fractional part to the result. See issue #14.

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

Rename method names
- makeGrayscale -> greyscale
- lightenColor -> lighten
- darkenColor -> darken

See #66
@eunomie

This comment has been minimized.

Show comment
Hide comment
@eunomie

eunomie Oct 27, 2015

Contributor

I've made some big changes.
I use HSL to manipulate colors, it's really more easier. And this is compatible with Less.
I've extracted all common methods in a base class. I've firstly done the same thing with HSB methods, it's no more necessary but still there. Would you I remove this change?

Contributor

eunomie commented Oct 27, 2015

I've made some big changes.
I use HSL to manipulate colors, it's really more easier. And this is compatible with Less.
I've extracted all common methods in a base class. I've firstly done the same thing with HSB methods, it's no more necessary but still there. Would you I remove this change?

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

@eunomie

This comment has been minimized.

Show comment
Hide comment
@eunomie

eunomie Oct 27, 2015

Contributor

I've added a spin function. Really useful to have analogous or triad colors from a base color. Spin is a rotation of hue, like a rotation in a color wheel.

Contributor

eunomie commented Oct 27, 2015

I've added a spin function. Really useful to have analogous or triad colors from a base color. Spin is a rotation of hue, like a rotation in a color wheel.

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

Rename method names
- makeGrayscale -> greyscale
- lightenColor -> lighten
- darkenColor -> darken

See #66

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 27, 2015

@iflan

This comment has been minimized.

Show comment
Hide comment
@iflan

iflan Oct 28, 2015

Member

Hi Yves,

I haven't had a chance to look at this today. I'll take another pass at it tomorrow.

Ian

Member

iflan commented Oct 28, 2015

Hi Yves,

I haven't had a chance to look at this today. I'll take another pass at it tomorrow.

Ian

@@ -28,6 +29,7 @@
public static final int H = 0;
public static final int S = 1;
public static final int B = 2;
+ public static final int L = 2;

This comment has been minimized.

@iflan

iflan Oct 30, 2015

Member

I presume this is lightness, right?

@iflan

iflan Oct 30, 2015

Member

I presume this is lightness, right?

This comment has been minimized.

@eunomie

eunomie Oct 30, 2015

Contributor

Yes, that's it.
I'll add a comment before each (and to explain why B and L are both '2')

@eunomie

eunomie Oct 30, 2015

Contributor

Yes, that's it.
I'll add a comment before each (and to explain why B and L are both '2')

@@ -43,6 +45,39 @@ public static Color hsbToColor(float[] inputHsb) {
return Color.getHSBColor(inputHsb[H], inputHsb[S], inputHsb[B]);
}
+ public static float[] hsbToHsl(float[] inputHsb) {

This comment has been minimized.

@iflan

iflan Oct 30, 2015

Member

Since the class is package-private (default visibility), it doesn't really make sense to make the methods be public. Can you remove the public from your methods and the existing ones?

And can you add some JavaDoc?

@iflan

iflan Oct 30, 2015

Member

Since the class is package-private (default visibility), it doesn't really make sense to make the methods be public. Can you remove the public from your methods and the existing ones?

And can you add some JavaDoc?

+ return hsbToHsl(toHsb(color));
+ }
+
+ public static Color hslToColor(float[] inputHsl) {

This comment has been minimized.

@iflan

iflan Oct 30, 2015

Member

This is basically hsbToColor(hslTohsb(inputHsl)), so why not just supply hslTohsb()?

@iflan

iflan Oct 30, 2015

Member

This is basically hsbToColor(hslTohsb(inputHsl)), so why not just supply hslTohsb()?

This comment has been minimized.

@eunomie

eunomie Oct 30, 2015

Contributor

I prefer read in caller code hslToColor(hsl) than hslToColor(hslToHsb(hsl)) which can be an implementation detail.
But I can add a hslToHsb and change hslToColor to call hslToHsb.
So the code of ColorUtil will be more symmetric:

  • hsbToHsl and hslToHsb
  • toHsb and toHsl
  • hsbToColor and hslToColor
@eunomie

eunomie Oct 30, 2015

Contributor

I prefer read in caller code hslToColor(hsl) than hslToColor(hslToHsb(hsl)) which can be an implementation detail.
But I can add a hslToHsb and change hslToColor to call hslToHsb.
So the code of ColorUtil will be more symmetric:

  • hsbToHsl and hslToHsb
  • toHsb and toHsl
  • hsbToColor and hslToColor
@@ -459,6 +467,454 @@ public Color addValuesToHsbComponents(Color baseColor,
}
+ /**
+ * Base implementation of color manipulation functions in HSL color space

This comment has been minimized.

@iflan

iflan Oct 30, 2015

Member

How about:

Abstract base class providing HSL color space manipulation functions.
@iflan

iflan Oct 30, 2015

Member

How about:

Abstract base class providing HSL color space manipulation functions.
+ + " %s, %s, %s, %s. ",
+ baseColorString, hueToAdd, saturationToAdd, lightnessToAdd);
+ throw new GssFunctionException(message);
+ } catch (IllegalArgumentException e) {

This comment has been minimized.

@iflan

iflan Oct 30, 2015

Member

If you want to use Java 7's multi-catch to merge the two catch blocks, I'm OK with that.

@iflan

iflan Oct 30, 2015

Member

If you want to use Java 7's multi-catch to merge the two catch blocks, I'm OK with that.

@@ -204,6 +204,54 @@ public void testMakeTranslucent_otherLocale() {
}
}
+ public void testSaturateColor() throws GssFunctionException {

This comment has been minimized.

@iflan

iflan Oct 30, 2015

Member

Can you add a test or two that exercises the wrap-around behavior of hue and the clipping behavior of saturation and lightness?

@iflan

iflan Oct 30, 2015

Member

Can you add a test or two that exercises the wrap-around behavior of hue and the clipping behavior of saturation and lightness?

This comment has been minimized.

@eunomie

eunomie Oct 30, 2015

Contributor

Yeah, sure.

@eunomie

eunomie Oct 30, 2015

Contributor

Yeah, sure.

@iflan

This comment has been minimized.

Show comment
Hide comment
@iflan

iflan Oct 30, 2015

Member

Awesome work! Please let me know when I should look again.

Also, because this is a big change, I'm going to send this through internal review and commit it there, then export it back to this repo. I'll do my utmost to make sure that the change still comes out as authored by you. :-)

Member

iflan commented Oct 30, 2015

Awesome work! Please let me know when I should look again.

Also, because this is a big change, I'm going to send this through internal review and commit it there, then export it back to this repo. I'll do my utmost to make sure that the change still comes out as authored by you. :-)

@eunomie

This comment has been minimized.

Show comment
Hide comment
@eunomie

eunomie Oct 30, 2015

Contributor

Thanks for all your interesting comments! I'll check tonight or in the weekend and I'll add a comment when everything is ready on my side.

Contributor

eunomie commented Oct 30, 2015

Thanks for all your interesting comments! I'll check tonight or in the weekend and I'll add a comment when everything is ready on my side.

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 30, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 30, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 30, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 30, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 30, 2015

eunomie added a commit to eunomie/closure-stylesheets that referenced this pull request Oct 30, 2015

@eunomie

This comment has been minimized.

Show comment
Hide comment
@eunomie

eunomie Nov 2, 2015

Contributor

I've added some comments (hope they are comprehensible ;)) and tests.
I've made 2 PR for try/catch and public/package remarks.

Contributor

eunomie commented Nov 2, 2015

I've added some comments (hope they are comprehensible ;)) and tests.
I've made 2 PR for try/catch and public/package remarks.

@@ -25,9 +26,22 @@
*/
class ColorUtil {
+ /**
+ * Index of Hue in HSB and HSL array.

This comment has been minimized.

@iflan

iflan Nov 2, 2015

Member

BTW, if you want to make this a one-line JavaDoc, that's fine with me:

/** Index of Hue in HSB and HSL array. */
@iflan

iflan Nov 2, 2015

Member

BTW, if you want to make this a one-line JavaDoc, that's fine with me:

/** Index of Hue in HSB and HSL array. */

This comment has been minimized.

@eunomie

eunomie Nov 2, 2015

Contributor

I've updated this part.

@eunomie

eunomie Nov 2, 2015

Contributor

I've updated this part.

@eunomie

This comment has been minimized.

Show comment
Hide comment
@eunomie

eunomie Nov 2, 2015

Contributor

The branch is force-pushed against new master (and fix conflicts in ColorUtil)

Contributor

eunomie commented Nov 2, 2015

The branch is force-pushed against new master (and fix conflicts in ColorUtil)

@iflan

This comment has been minimized.

Show comment
Hide comment
@iflan

iflan Nov 3, 2015

Member

Yves, I'm getting this change reviewed internally today.

Member

iflan commented Nov 3, 2015

Yves, I'm getting this change reviewed internally today.

@iflan

This comment has been minimized.

Show comment
Hide comment
@iflan

iflan Nov 6, 2015

Member

This is merged with 5e681fe.

Member

iflan commented Nov 6, 2015

This is merged with 5e681fe.

@iflan iflan closed this Nov 6, 2015

@eunomie

This comment has been minimized.

Show comment
Hide comment
@eunomie

eunomie Nov 6, 2015

Contributor

Yeah! Thanks a lot!

Contributor

eunomie commented Nov 6, 2015

Yeah! Thanks a lot!

@iflan

This comment has been minimized.

Show comment
Hide comment
@iflan

iflan Nov 6, 2015

Member

Thank you. BTW, I did make a few changes based on our internal review, but they were minor. :-)

Member

iflan commented Nov 6, 2015

Thank you. BTW, I did make a few changes based on our internal review, but they were minor. :-)

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