bgrins / TinyColor

Fast, small color manipulation and conversion for JavaScript
https://bgrins.github.io/TinyColor/
MIT License
5.08k stars 438 forks source link

feat: add random as a static member of TinyColor #175

Closed scttcper closed 6 years ago

bgrins commented 6 years ago

This got to my expected behavior - thanks! But I don't think it's working exactly how I expected it would. What I expected:

What I think is currently happening:

The reason I expected the former behavior is that so when we load the UMD package - for example, by doing:

echo '<script src="tinycolor.umd.min.js"></script>' > dist/bundles/index.html && open dist/bundles/index.html

Then in the console in that page, if I do tinycolor.random() it works as expected (matching v1 API). If I do new tinycolor("red") or tinycolor("red") it throws errors (since tinycolor is just a plain object. If instead 'TinyColor' was exported, we could do all of these (I think - please correct me if I'm wrong): TinyColor.random(), new TinyColor("red"), TinyColor("red").

scttcper commented 6 years ago

okay i think what we're looking for is something like this then. Typescript sure doesn't like it very much. I'd never done an export like this before, learn something new every day.

This was my small test.

class Test {
  constructor() { console.log('created test') }
  go() { console.log('go') }
}

var _old = Test;
Test = function(...args) { return new _old(...args) };

// can be called either way
Test().go()
new Test().go()
codecov-io commented 6 years ago

Codecov Report

Merging #175 into v2 will increase coverage by 0.16%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##              v2     #175      +/-   ##
=========================================
+ Coverage   98.4%   98.56%   +0.16%     
=========================================
  Files         10       10              
  Lines        626      628       +2     
  Branches     150      150              
=========================================
+ Hits         616      619       +3     
+ Misses        10        9       -1
Impacted Files Coverage Δ
src/index.ts 97.73% <100%> (+0.02%) :arrow_up:
src/random.ts 97.27% <0%> (+0.9%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 96aeff9...6500026. Read the comment docs.

bgrins commented 6 years ago

Yeah TS doesn't seem too happy about it. I think it's nice to have the backwards-compat syntax without new, but I think it'd also be OK to drop the non-new format and just export the class if we tack on random, etc as statics. As far as doing that - it seems like that's relatively straightforward as static random = random; (assuming that random it imported at the top of index.ts), right?

bgrins commented 6 years ago

but I think it'd also be OK to drop the non-new format and just export the class if we tack on random, etc as statics.

Specifically, supporting:

TinyColor.random();
new TinyColor("red");
scttcper commented 6 years ago

Revered the optional new tinycolor and added random as a member of the TinyColor class.

bgrins commented 6 years ago

Great, thanks! Could you also expose readability, fromRatio, legacyRandom, toMsFilter` in the same manner? This would allow us to remove most of the "several functions" section in the migration guide at https://github.com/bgrins/TinyColor/blob/01caecf94411443d020310dbd724cde38669f13d/README.md#changes-from-v1.