Closed le717 closed 9 years ago
Yes, but since you added tinycolor.getOriginalInput(), I think it changes expectations. I would expect that method to be called something like toNormalizedString().
So the review suggestion is that toString
is renamed to toNormalizedString
and then rename getOriginalInput
to toString
? I think changing the behavior of toString
wouldn't make sense, since it can take in formats ("hex", "hsl", etc) where the original string has no meaning: https://github.com/bgrins/TinyColor#tostring.
and/or rename getOriginalInput() to something that better describes it's actions vs toString()
Did you have anything in mind?
@bgrins It just seems like toString()
should detect when _originalInput
is all upper case and respect that.
It just seems like toString() should detect when _originalInput is all upper case and respect that.
Given that rule, I see:
tinycolor("red").toString()
: red
tinycolor("RED").toString()
: RED
tinycolor("#FF0000").toString()
: #FF0000
How about given this input?
tinycolor("RED").toString("rgb")
: rgb(255, 0, 0)
or RGB(255, 0, 0)
?tinycolor("ReD").toString()
: red
, ReD
, or RED
?In Brackets, we have a preference for uppercase if that's true, then it's converted to UPPER, otherwise conversions use lower.
So, my biased opinion :) would be that if all alpha chars in original are uppercase use UPPER, otherwise lower.
tinycolor("RED").toString("rgb")
:rgb(255, 0, 0)
orRGB(255, 0, 0)
?
RGB(255, 0, 0)
tinycolor("ReD").toString()
:red
,ReD
, orRED
?
red
What about adding another parameter to toString()
, say, uppercase
, that when set to true
it returns the content in uppercase (default false
, lowercase). That might be easier to implement than upper/lowercase detection and return (which is trivial in itself anyway).
It's the API design that I'm struggling with here.
I'm afraid implementing this behavior by default in toString
would be surprising behavior (if a string is parsed as the same color/format I would expect that calling toString() on it would always return the same thing regardless of the original input). I think this 'expected behavior' may be the point of disagreement.
There could be an extra param to toString
or a new function that follows this behavior, but neither option really seems right to me. Which makes me think this may be an application-specific problem/solution. That's my current thinking, at least.
Also, this could be implemented easily outside of the library:
var color = tinycolor("RED");
var str = color.toString();
if (color.getOriginalInput().toUpperCase() === color.getOriginalInput()) {
str = str.toUppercase();
}
or
tinycolor.prototype.toStringPreservingCase = function() {
if (this.getOriginalInput().toUpperCase() === this.getOriginalInput()) {
return this.toString().toUpperCase();
}
return this.toString();
}
Also, this could be implemented easily outside of the library:
I guess maybe not that easy - it would also need to make sure that getOriginalInput is a string before calling toUppercase. But I guess in the case of object input it should default to lower.
Also, this could be implemented easily outside of the library:
I had something initially very similar to your examples implemented in the code. It might be better for it to be done that way anyway. I reported this as it was a source of confusion for both me and @redmunds, and I didn't know if this would be something you'd be interested in.
@bgrins No worries. Thanks for listening to my opinion.
@le717 @redmunds Thanks for the help on TinyColor and the great work on Brackets :). Going to close this, but feel free to reopen if you'd like to discuss further.
In having adobe/brackets#9596 reviewed, @redmunds brought out an interesting observation:
I am wondering what your thoughts are on this, @bgrins. I know
toString()
is a long-standing method and removing/renaming it would be a huge breaking change, but would it be possible to better document the normalization it performs and/or renamegetOriginalInput()
to something that better describes it's actions vstoString()
? I mention renaming as it is a very recent edition to TinyColor and probably isn't used much yet.