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

Version 1.x Makeover (Rewrite + New Features + Bugfixes) #96

Merged
merged 1 commit into from
Dec 3, 2016
Merged

Conversation

Qix-
Copy link
Owner

@Qix- Qix- commented Nov 16, 2016

MAKEOVER WITH RAINBOWS AND COLORS AND SHIT

Documentation will be updated after API is finalized. Please be mindful of this!

Here it is, the final piece of the color makeover trilogy. If you recall, I did a makeover of both color-string and color-convert that have been generally well-received. This was the final piece.

After an increasing number of people expressing some stress over the 'quirkiness' of the API recently, I decided to finally get to it. It's been about an 8 or 9 hour ordeal, which is why I was really good at procrastinating 👯‍♂️.

I do already have some things in mind for a 2.x makeover, but that's far into the future and is contingent on the evolution of color-convert specifically.

Without further adieu, here are all of the changes. Keep in mind this is a complete re-write with a pretty good portion of the API being compatible with the 0.x releases.

The Fun Parts (new stuff, fixes, etc.)

Color(0xFF0000).rgb().hsl().lab().hwb().xyz().cmyk().ansi256().rgb().toString()

Yes, this does mean that portion of the API has changed. Please see the migrations section below.

As well, bumping the dependencies fixes #74.

The Not-So-Fun Parts (migration)

  • Everything is immutable. There isn't a single fix for this if you (ab)used this library earlier on - you'll most likely be tweaking several touch points. It's for the greater good, I promise. If you already treated the library as immutable then you're probably good to go. Good tests are key here.
  • All .model() functions (to convert to an object) must now be .model().object(). See notes below for justification.
  • All .modelArray() functions must now be .model().array(). See notes below for justification.
  • When using the .object() method, alpha values are now .alpha instead of .a so as to not conflict with Lab.
  • If you used the internal properties of the object instead of getters, you'll need to.. not do that anymore :) Use the respective model calls (.rgb(), .hsl(), etc.) paired with .array() - this is because we don't store every single model in the object at once, but simply the last worked-with model.
  • .whiteness() / .blackness() -> .white() / .wblack() (note the w prefix).

I want to explain this one because it doesn't feel 'great' but I felt like it needed to happen. We have had a couple of issues (#73 being the most definitive) that address problems with the assumed functionality of the methods.

I chose to remove the -ness from the names because I didn't want them being interpreted as a characteristic of the color, but instead a more concrete, 'absolute' (if you will) property of a specific color model (akin to .cyan(), .red(), .hue(), etc.).

The w prefix was chosen for both objective and subjective reasons: it objectively conflicts with CMYK's .black(), and HWB subjectively isn't used as much as CMYK is (please let me know if this is an abhorrently wrong assumption). Further, we have another example of the API doing this that nobody has complained about since I took over - .saturationl() and .saturationv() for the HSV and HSL models.

Also, since this seems like a good place to mention it - in lieu of something like .lightnessab() for the Lab model, I just stuck with .l(). In the future, I hope to have something a little more consistent, but for now this works.

  • Constructors with specialized keys (such as .red, .green and .blue for RGB instead of .r, .g. and .b) were removed. If this is a huge problem, please let me know. For clarity, this means you can no longer instantiate with Color({red: 10, green: 100, blue: 150}) but instead must use Color({r: 10, g: 100, b: 150}).
  • The model getter methods (e.g. .rgb(), .hsl(), .keyword()) no longer act as setters. Since everything is immutable now, such functionality is useless and only creates more overhead. Please use the Color() constructor directly, or use the new 'static' methods (functions) such as Color.rgb() directly.
  • (Closes Color.clone doesn't create equal object #67, '.mix()' method fails to work with cloned color #46) .clone() was removed since all objects are immutable. Just pass objects directly. If you really need a new copy of an object, use Color(otherColorObject).
  • rgbaString() was removed. If you really need a string that forces the alpha value, please let me know and I can add something into the API. Otherwise, use .rgbString() and if the alpha is anything but 100% then it will include it.
  • The specialized .xxxString() methods should move to .xxx().string() calls. In the case you need the RGB percent string, you may still use .percentString() - it will automatically convert to the RGB model. For instance, if you used .hslString(), you would now call .hsl().string().
  • .clearer() -> .fade(), because "clearer" is not at all what I thought it was personally and the API is already kind of confusing (I thought clearer meant more saturated).
  • .toJSON() now returns the current model's representation. If you absolutely need RGB, tack on .rgb(). This shouldn't affect many people, if any.
  • Values no longer normalize (round) when getting objects or arrays. Call .round() first. If you are seeing crazy long decimals, this is why. I wanted to be sensitive to the scientific community that is using this library to perform more mathematically sensitive operations instead of approximate stuff for web users. Most people won't have a problem with this since most people use the CSS strings.
  • (Fixes Replace max values by modulo for degree #24) Initializing a Color object with a hue-based model (hsl, hwb, etc.) performs a rotation on the value instead of clamping it to 0-360. This is in-line with the .hue() getter/setter and I believe it to be the correct functionality to begin with.
  • .greyscale() -> .grayscale() - if you think we should have both, please let me know :)
  • (Closes Aliasing & Preventing Surprises #86) .saturation() -> .saturationl() (to match .saturationv())

Some Notes

So why the extra three characters for things like .hslString() -> .hsl().string()? Two reasons - one, you can now snap colors to another model if necessary. For example, if I wanted to snap a color to the nearest ANSI 256 color, I can do Color(123, 44, 220).ansi256().rgb().object(); two, the dependencies (e.g. color-string) have pretty good parity with the model names used in color-convert, and color-convert has evolved a bit since the 0.x releases of color to a point where we can intelligently include most models, and can future-proof new models just by updating color-convert in color's package.json.

Further, a lot of these changes are due to the internal object no longer holding every single conversion possible. color-convert now supports quite a lot of conversion routes and maintaining all of them here was impractical, messy and tedious.

As mentioned earlier, another win is that you're going to save quite a bit of processing time, so applications that are doing things like color manipulation on images and whatnot should benefit greatly by switching to 1.x.

Next Steps

After this PR is approved by the community and merged, it will be published as 1.0.0.

After that, there will be a few efforts moving forward:

  • Tests (of course) - I noticed comprehensive tests are lacking, and a test makeover would be beneficial in its own right.
  • More transformations, and more clarity as to what those transformations are.
  • Color scheme support (Add schemas #44)
  • Color space support (maybe)

I think the first thing that should be tackled now that we will have a slightly cleaner code base is to figure out some of the hairy naming that is going on here. I would love to hear some ideas about this (feel free to drop as many tickets as you'd like) - I have some of my own, but I don't always have the best ideas!

Feedback!

I need feedback on this as I am not a primary user of this library (admittedly I use color-convert more than color proper) so I want to make sure the users of this library are satisfied before I merge it in. There are a lot of opinionated changes in here that I have included based on all of the tickets and pull requests I've received.

Another thing I'd like to point out - this library is edge-case hell. Making decisions that benefitted all users, use cases, and color models, elegantly and without a ton of overhead, was incredibly difficult. I've attempted this rewrite probably 4 or 5 times now, ultimately scrapping it and trying again a month or two later.

If you have any strong feelings, or even not-so-strong feelings, please let me know. I'd love to know what could be done better.

Obligatory CC list:
// @MoOx @sindresorhus @kevinSuttle @mattbasta @xml @huan086 @ooflorent @mindjuice @wmira @Lapanoid @ZuBB @iamstarkov @stevemao @ericclemmons @igl @dliv

Thanks everybody!

@Qix- Qix- added this to the 1.0.0 (Makeover) milestone Nov 16, 2016
@Qix- Qix- changed the title Version 1.x Makeover (Rewrite + New Features + Bugfixes) 💄 Version 1.x Makeover (Rewrite + New Features + Bugfixes) Nov 16, 2016
@Qix- Qix- changed the title 💄 Version 1.x Makeover (Rewrite + New Features + Bugfixes) Version 1.x Makeover (Rewrite + New Features + Bugfixes) Nov 16, 2016
@mattbasta
Copy link

Pedantry: you should consider using trailing commas in your array and object literals.

this.valpha = typeof obj[channels] === 'number' ? obj[channels] : 1;
} else if (typeof obj === 'number') {
// this is always RGB - can be converted later on.
obj &= 0xFFFFFF;

Choose a reason for hiding this comment

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

It may be worthwhile to also support an alpha channel, though I'm not sure if this is possible with hex numbers, leaning towards "probably no."

https://drafts.csswg.org/css-color/#hex-notation

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah there have been discussions about both RGBA and ARGB. Also, if they leave it off, it translates to 0. Does that mean 0 alpha, or a default of 1? There was no was to intelligently tell without it being a string or mandating that alpha is expected.

Choose a reason for hiding this comment

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

Not sure if this is helpful or not, but ES2015 added support for octals. https://babeljs.io/docs/learn-es2015/#binary-and-octal-literals

Copy link
Owner Author

Choose a reason for hiding this comment

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

Octals are awful, I personally refuse to use them. Plus they wouldn't help here since I still get an integer passed in.

];
this.valpha = 1;
} else {
this.valpha = 1;

Choose a reason for hiding this comment

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

may be worthwhile to declare this once at the top and reassign it in each condition where it could be something other than 1

this.valpha = typeof obj.alpha === 'number' ? obj.alpha : 0;
}

var hashedKeys = keys.sort().join('');

Choose a reason for hiding this comment

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

This seems kind of brittle, no? I'd at least choose a delimiter

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fair enough, yeah.

it('Immutability', function () {
var c = Color(0xFF0000);
ok(c !== c.rgb());
ok(c != c.rgb()); // eslint-disable-line eqeqeq
});

Choose a reason for hiding this comment

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

What does this actually test?

Copy link
Owner Author

Choose a reason for hiding this comment

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

In the old verson, these would have returned that they were equal.

@ZuBB
Copy link
Contributor

ZuBB commented Nov 16, 2016

Color(0xFF0000).rgb().hsl().lab().hwb().xyz().cmyk().ansi256().rgb().toString()
vs
.xxx().string()

I did not look into the code yet, but does they (string() vs toString()) do same job?

@mattbasta
Copy link

As a matter of guaranteeing immutability and perhaps protecting the users from themselves, you might want to use Object.freeze() on this and this.color in the Color constructor

@Qix-
Copy link
Owner Author

Qix- commented Nov 16, 2016

@mattbasta: Pedantry: you should consider using trailing commas in your array and object literals.

I use the XO coding style which doesn't allow them. :)

@mattbasta
Copy link

You can override that, no? Avoiding them was a Crockfordism, IIRC, because IE didn't parse JS correctly until IE8 or 9. It's really a joy to write. And ES2017 has trailing comma support for function arguments, too.

@Qix-
Copy link
Owner Author

Qix- commented Nov 16, 2016

@ZuBB: I did not look into the code yet, but does they (string() vs toString()) do same job?

Yes, though toString() is a special method in Javascript. :) Since camel case convention isn't common throughout the API I decided to keep them as two; string for explicit calls and leaving toString available for the Javascript language.

@Qix-
Copy link
Owner Author

Qix- commented Nov 16, 2016

@mattbasta / cc @sindresorhus on that one (Sindre created XO). I tend to be against overriding XO rules though I'd agree that if there's a functional benefit then it should be observed.

However, I don't personally support IE 7 or 8 in anything I do, and if that is the only objective gain then I'd still err on the side of not having them.

@ZuBB
Copy link
Contributor

ZuBB commented Nov 16, 2016

You can override that, no? Avoiding them was a Crockfordism, IIRC, because IE didn't parse JS correctly until IE8 or 9. It's really a joy to write. And ES2017 has trailing comma support for function arguments, too.

I am ought to support 1 project for IE8. @mattbasta can you suggest any "compiler" that works "down"?

@mattbasta
Copy link

@ZuBB I'm not sure sure what you mean

@ZuBB
Copy link
Contributor

ZuBB commented Nov 16, 2016

lets assume we added that trailing comma. what tool I need to use to get this lib working in IE8?

my first guess was babel. but, honestly i do not have any experience with it. so I do not know if it can drop that trailing commas..

@Qix-
Copy link
Owner Author

Qix- commented Nov 16, 2016

I misread your comment, @mattbasta; that should be fine. I'm not sure what the argument is then since those browsers support non-trailing commas (as they should - could you imagine if they didn't?).

@mattbasta
Copy link

@ZuBB Any minifier will remove them. Uglify, Closure Compiler, etc. Babel will probably leave them in, actually.

@Qix- A few nice things: a.) you'll never need to fiddle with commas when moving lines around b.) you'll never accidentally paste something in and have it be a syntax error, since everything is consistent and the same c.) nerdy opinions

@mattbasta
Copy link

WRT this PR, I think it looks v.good overall

@Qix-
Copy link
Owner Author

Qix- commented Nov 16, 2016

@mattbasta I froze the objects :) Just required tweaking .alpha() a bit. Good tip!

@Qix-
Copy link
Owner Author

Qix- commented Nov 16, 2016

Side note: I'm amazed at how well diff did at figuring out how things transformed even though it's a complete rewrite.

@ericclemmons
Copy link

Another thing I'd like to point out - this library is edge-case hell. Making decisions that benefitted all users, use cases, and color models, elegantly and without a ton of overhead, was incredibly difficult.

Complete empathy for you, and your rationale / decisions make a lot of sense. (For me to argue in favor of changes right now would be as silly as comma placements :D).

What's the best way to test this out in an existing project? Is there something npm install-able, or is there a better way?

@Qix-
Copy link
Owner Author

Qix- commented Nov 18, 2016

@ericclemmons

Complete empathy for you, and your rationale / decisions make a lot of sense.

Thank you, and glad to hear :)

What's the best way to test this out in an existing project? Is there something npm install-able, or is there a better way?

$ npm i Qix-/color#makeover

:)

@ericclemmons
Copy link

@Qix- I'm glad that .toString() is there, as I'm using color with styled-components, which means the colors should be automatically cast correctly.

@Qix-
Copy link
Owner Author

Qix- commented Nov 18, 2016

@ericclemmons Everything working okay otherwise? :)

@sindresorhus
Copy link
Sponsor

sindresorhus commented Nov 20, 2016

Generally looks good :)


Main Color class now has swizzle-able model constructors (e.g. Color.cmyk(10, 20, 30, 40), Color.rgb([15, 67, 200]), etc.).

Not a big fan of this. We now have rest/spread, so I don't really see the use-case of this either. Pick one and stick with it. Either can be easily used. For example, if you only support an array, users can just do: Color.cmyk(...array).

@mindjuice
Copy link

@sindresorhus Do I misunderstand you or did you mean:

For example, if you don't support passing an array, users can just do Color.cmyk(...array).

Regarding the use of spreads, not everyone is using ES2015 yet, so having these functions take an arguments list or an array still seems valuable for some people (not me...I'm using ES2015, but I know some folks who can't use it on existing projects).

The downside of adding support for both styles to the API now is that it will be hard to remove one of them later, even when "everyone" is using ES2015.

I suppose you could document that this library is mainly targeted at ES2015 users, but then it would be weird that it's not written in ES2015. 😄

On the other hand, if you prefer to choose one style, I think accepting individual arguments seems like the winner. Having to pass all arguments in an array to every call isn't very nice. Choosing one style also removes the overhead of the swizzle function on every Color constructor.

@sindresorhus
Copy link
Sponsor

@mindjuice I think this module should look forward. Users not using ES2015 can always use apply:

Color.cmyk.apply(Color, array);

I don't think it's very common to pass an array regardless.

@mindjuice
Copy link

@sindresorhus I was going to mention apply, but it's kind of ugly (perhaps the price you pay for being "stuck" on ES5).

Agreed that it's probably an unusual situation to be using ES5 and have your args in an array, so I'm on board with just allowing arguments to be passed separately as I mentioned above.

Maybe this module should be written in ES2015 to be forward-looking too.

@Qix-
Copy link
Owner Author

Qix- commented Nov 21, 2016

@mindjuice I know of a lot of projects at a few companies that rely on this module and are still forced to use 0.10 or 0.12, and babel is annoying to set up just for a few dependencies. I'll probably stick to ES5, though I agree that swizzle isn't all that necessary.

@mindjuice
Copy link

@Qix- You don't need to distribute the code in ES2015 format. Lots of projects do a build and then just distribute the generated ES5 on npm.

Leaving it ES5 though is totally fine though, especially since you've already done the work (thanks for that BTW!). 😄

@ericclemmons
Copy link

@Qix- I removed `.clone()` and `.hexString()` from my code and the colors + stories (we use React Storybook) worked the same!
diff --git a/package.json b/package.json
index ca2d601..437c27b 100644
--- a/package.json
+++ b/package.json
@@ -92,7 +92,7 @@
     "body-parser": "1.15.2",
     "classnames": "2.2.5",
     "clipboard": "1.5.12",
-    "color": "0.11.4",
+    "color": "github:qix-/color#makeover",
     "common-tags": "1.3.1",
     "compression": "1.6.2",
     "connect-redis": "3.1.0",
diff --git a/src/components/QuickDegreeFinder/Button.js b/src/components/QuickDegreeFinder/Button.js
index a900f50..2e17f39 100644
--- a/src/components/QuickDegreeFinder/Button.js
+++ b/src/components/QuickDegreeFinder/Button.js
@@ -4,7 +4,7 @@ import contrast from "./contrast";
 import eh from "./eh";

 export default styled.button`
-  background: ${(props) => props.theme.ctaColor.hexString()};
+  background: ${(props) => props.theme.ctaColor};
   border: none;
   border-radius: 3px;
   box-shadow: 0 ${eh(-1)}px 0 rgba(0, 0, 0, 0.2) inset;
@@ -22,6 +22,6 @@ export default styled.button`
   }

   &:hover {
-    background: ${(props) => props.theme.ctaColor.clone().darken(0.2).hexString()};
+    background: ${(props) => props.theme.ctaColor.darken(0.2)};
   }
 `;
diff --git a/src/components/QuickDegreeFinder/Fieldset.js b/src/components/QuickDegreeFinder/Fieldset.js
index c113db6..06ed8f3 100644
--- a/src/components/QuickDegreeFinder/Fieldset.js
+++ b/src/components/QuickDegreeFinder/Fieldset.js
@@ -1,7 +1,7 @@
 import styled from "styled-components";

 export default styled.fieldset`
-  background: ${(props) => props.theme.backgroundColor.hexString()};
+  background: ${(props) => props.theme.backgroundColor};
   border: none;
   border-bottom: 1px solid rgba(0, 0, 0, 0.15);
   box-sizing: border-box;
diff --git a/src/components/QuickDegreeFinder/Form.js b/src/components/QuickDegreeFinder/Form.js
index 25f24de..93ef3dc 100644
--- a/src/components/QuickDegreeFinder/Form.js
+++ b/src/components/QuickDegreeFinder/Form.js
@@ -4,11 +4,11 @@ import eh from "./eh";

 export default styled.form`
   align-items: stretch;
-  background: ${(props) => props.theme.backgroundColor.hexString()};
+  background: ${(props) => props.theme.backgroundColor};
   background-image: ${(props) => `
     linear-gradient(
-      ${props.theme.backgroundColor.hexString()} 0px,
-      ${props.theme.backgroundColor.clone().darken(0.1).hexString()} 100%
+      ${props.theme.backgroundColor} 0px,
+      ${props.theme.backgroundColor.darken(0.1)} 100%
     )
   `};
   border-radius: 3px;
diff --git a/src/components/QuickDegreeFinder/Legend.js b/src/components/QuickDegreeFinder/Legend.js
index 451bf18..45ab11c 100644
--- a/src/components/QuickDegreeFinder/Legend.js
+++ b/src/components/QuickDegreeFinder/Legend.js
@@ -4,7 +4,7 @@ import contrast from "./contrast";
 import eh from "./eh";

 export default styled.legend`
-  background: ${(props) => props.theme.titleColor.hexString()};
+  background: ${(props) => props.theme.titleColor};
   border-top-left-radius: 3px;
   border-top-right-radius: 3px;
   box-sizing: border-box;

@Qix-
Copy link
Owner Author

Qix- commented Nov 21, 2016

@mindjuice That's fair. I might consider doing that in the future :)

@ericclemmons glad to hear :D 🎉 💃

I might remove the swizzle support since if someone really needs it later we can make an additive change instead of a breaking change to remove it.

@Qix-
Copy link
Owner Author

Qix- commented Dec 3, 2016

Thank you everyone!

:lipstick:

@Qix-
Copy link
Owner Author

Qix- commented Dec 3, 2016

Published as 1.0.0!

@ajoslin
Copy link

ajoslin commented Dec 3, 2016

Immutability + toString() is all I care about, and it will make life a lot easier. Thank @Qix-!

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