-
Notifications
You must be signed in to change notification settings - Fork 267
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
Conversation
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; |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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(''); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 | ||
}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I did not look into the code yet, but does they ( |
As a matter of guaranteeing immutability and perhaps protecting the users from themselves, you might want to use |
I use the XO coding style which doesn't allow them. :) |
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. |
Yes, though |
@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. |
I am ought to support 1 project for IE8. @mattbasta can you suggest any "compiler" that works "down"? |
@ZuBB I'm not sure sure what you mean |
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.. |
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?). |
@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 |
WRT this PR, I think it looks v.good overall |
@mattbasta I froze the objects :) Just required tweaking |
Side note: I'm amazed at how well diff did at figuring out how things transformed even though it's a complete rewrite. |
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 |
Thank you, and glad to hear :)
$ npm i Qix-/color#makeover :) |
@Qix- I'm glad that |
@ericclemmons Everything working okay otherwise? :) |
Generally looks good :)
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: |
@sindresorhus Do I misunderstand you or did you mean:
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 |
@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. |
@sindresorhus I was going to mention 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. |
@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. |
@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!). 😄 |
@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; |
@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. |
Published as |
Immutability + toString() is all I care about, and it will make life a lot easier. Thank @Qix-! |
Here it is, the final piece of the color makeover trilogy. If you recall, I did a makeover of both
color-string
andcolor-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-convert
are supported (using.<model-name>()
). Try it yourself!Yes, this does mean that portion of the API has changed. Please see the migrations section below.
Color
class now has swizzle-able model constructors (e.g.Color.cmyk(10, 20, 30, 40)
,Color.rgb([15, 67, 200])
, etc.)..toString()
now returns a color string..round()
function that takes a number of places (default is 0) to round the internal values to. The modified.string()
and.percentString()
methods also take this parameter, and those methods default to 1. See notes below.As well, bumping the dependencies fixes #74.
The Not-So-Fun Parts (migration)
.model()
functions (to convert to an object) must now be.model().object()
. See notes below for justification..modelArray()
functions must now be.model().array()
. See notes below for justification..object()
method, alpha values are now.alpha
instead of.a
so as to not conflict with Lab..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 thew
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..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 withColor({red: 10, green: 100, blue: 150})
but instead must useColor({r: 10, g: 100, b: 150})
..rgb()
,.hsl()
,.keyword()
) no longer act as setters. Since everything is immutable now, such functionality is useless and only creates more overhead. Please use theColor()
constructor directly, or use the new 'static' methods (functions) such asColor.rgb()
directly..clone()
was removed since all objects are immutable. Just pass objects directly. If you really need a new copy of an object, useColor(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..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..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.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 :).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 doColor(123, 44, 220).ansi256().rgb().object()
; two, the dependencies (e.g.color-string
) have pretty good parity with the model names used incolor-convert
, andcolor-convert
has evolved a bit since the 0.x releases ofcolor
to a point where we can intelligently include most models, and can future-proof new models just by updatingcolor-convert
incolor
'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:
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 thancolor
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!