svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.04k stars 1.08k forks source link

Allow for random colour generation #791

Closed saivan closed 5 years ago

saivan commented 6 years ago

Add random option and randomize() method to SVG.Color. Should use lab space by default so wait for #790 first.

Fuzzyma commented 6 years ago

I think we dnt need a random option. Just calling the constructor without any parameters could result in a random color instead. Or do you think it should default to black?

var random = new SVG.Color() // done
saivan commented 6 years ago

Not a bad plan! I like that idea. But is there never a case where you would change an existing SVG.Color(), or do we treat them as though they were immutable?

Fuzzyma commented 6 years ago

Immutability is a thing which is on the list to discuss ("stuff to think about") :D. I am a fan of immutable data structures. However - it is almost always slower. But I think it might pay off.

Anyway: why would it not be possible to change a color? That change only affects the construction of the color object and nothing else. You still just can change the properties if the object, can't you?

saivan commented 6 years ago

You can change the properties, but how do you make those properties propagate? Eg: If I change the color property in a color object, does a fill that uses this color automatically update?

Fuzzyma commented 6 years ago

No - Ofc not. I would also not expect this to happen. You paassed a color to fill() - what you do after this should habe no effect at all. At least that's my strong opinion

caimen commented 6 years ago

I worked on a random color generator on chance js which allows you to do ranges of random colors, such as if you want only light colors or dark colors. Essentially it lets you send min and max values for RGBA. This allows you some flexibility in getting the kind of random colors you want. I really had needed this at the time to generate dark colors for light text or light text for dark backgrounds. I would say feel free to grab bits and pieces if you need. https://github.com/chancejs/chancejs/blob/master/chance.js Line 1204

Fuzzyma commented 6 years ago

Thanks! We will take a look :)

oparisy commented 6 years ago

My two cents: please don't generate a random color when the constructor receives no arguments. I do not expect this behavior and never encountered it in other libs. IMHO people will pull their hair not understanding why their "simple" code has a non-reproducible behavior...

I for one am all for explicit, fluent API such as a "random" method.

saivan commented 6 years ago

@oparisy you have a very good point! I think thats a good plan!

oparisy commented 6 years ago

@saivan Great! Re-reading your initial proposal, and having a look at your fluent API style, I guess a randomize() method would indeed be a better fit since it is a verb.

PierBover commented 6 years ago

Is anyone working on this? Otherwise I'd like to contribute.

saivan commented 6 years ago

You're welcome to have a crack :) of course! We always welcome prs

saivan commented 5 years ago

So I did this in #923