bgrins / TinyColor

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

Preserve case when calling `toString()` #64

Closed le717 closed 10 years ago

le717 commented 10 years ago

This is exactly what I was asking for in #63. This made my changes much easier. :)

le717 commented 10 years ago

@bgrins Looks like I need to add some tests for this. What do you suggest? Would the example in #63 work?

bgrins commented 10 years ago

What if we stored the original input on the object (called originalInput or similar), so that you/others could run whatever logic would fit their application? Because it feels like someone else may want to run some other similar kind of transformation based on the original input. So you could do something like this:

function getProperlyCasedColorString(color) {
  var output = color.toString();
  var isUpperCase = typeof color.originalInput !== "object" ? color === color.originalInput.toUpperCase() : false;

  if (isUpperCase) {
    output = output.toUpperCase();
  }

  return output;
}

getProperlyCasedColorString(tinycolor("RGB(1,2,3)")); // "RGB(1, 2, 3)"
getProperlyCasedColorString(tinycolor("Rgb(1,2,3)")); // "rgb(1, 2, 3)"
getProperlyCasedColorString(tinycolor("rgb(1,2,3)")); // "rgb(1, 2, 3)"
le717 commented 10 years ago

To clarify, the function you've given, this would be in the TinyColor script but not a method of the tinycolor object, correct? Such a function is rather impossible in the Brackets module I'm editing (see the linked PR from adobe/brackets). That is a good idea, but said module is rather a mess, with the tinycolor not always directly accessed. This function would only make it even more confusing. A straight same in-same out is only thing that will really work in its current state...

bgrins commented 10 years ago

To clarify, the function you've given, this would be in the TinyColor script but not a method of the tinycolor object, correct?

So in my proposal, the originalInput property would be added to the tinycolor object to support your use case, while the function getProperlyCasedColorString is just a function that I made up that would be added to your application (inside the Bracket module you are working with).

And you would use that custom function instead of calling color.toString(). I tried to make up that function so that it would match the logic of this PR as I understand it (if the string comes in fully uppercase then output it fully uppercase, otherwise do fully lowercase). I think I'm missing a bit of context with the brackets PR - would it work if you replaced this newColor assignment with the hypothetical getProperlyCasedColorString function (that would be added to the module)

The reason that I'm thinking that it makes sense to keep that logic outside of the library is that other people may have other application specific logic that requires different rules (or they may not want uppercase output ever and always want it to come out lower case).

le717 commented 10 years ago

Ah, I get what you mean. Been on mobile (and still am), so I haven't been able to explain much. :)

Yes, you understand the use case, and your idea makes sense. I'll try to look at it this afternoon and submit a new PR if it works (I think it will).

le717 commented 10 years ago

@bgrins I finally got a chance to test out your idea. It worked, albeit with some small changes. I'll submit a new PR with the originalInput property right away.