Use upper/lowercase colors in Inline Color Editor #9596

le717 · 2014-10-19T00:34:00Z

Features

Preserves case between value conversions and slider changes
Related code updated to be case-insensitive
New tests for conversion, with all existing tests passing
Also made some minor code style fixes

This code is rather confusing (as already documented), so it was a bit of a challenge to get this written up. I once spent 30 digging around in various places for a one-line fix.

Now for the issues present here:

This requires an update to TinyColor v1.1.0+, included with this PR. The creator refactored the library quite a bit since the v0.9.10 version currently used. TinyColor also unconditionally converts input to lowercase, which would make us have to keep track if the input is uppercase or not (which, in line with the previously documented issues, would be very hard because of the changing types). I have worked with Brian on the aforementioned patch to store the original input, which in turn makes this patch much easier to work out.

Mark-Simulacrum · 2014-12-21T16:19:31Z

The "U" toggle button doesn't seem appropriate for a button of it's type. Perhaps it should be separated from the RGBa, Hex, and HSLa button-container. @larz0 will probably have a better idea, though.

At the very least, I would perhaps make the button have more text than "U" as that seems rather odd. Another improvement would be having the button change state depending on the current state (U/L, for example (Uppercase/Lowercase)). Again, @larz0 will probably have better design suggestions.

Mark-Simulacrum · 2014-12-21T16:55:20Z

I also found what seems like a bug. Whenever I:

Edit a lowercase color (#ecc).
Change the color type to HSLa. (hsl(0, 50%, 87%))
Hit "U" to go uppercase. (HSL(0, 50%, 87%))
Change opacity to another value.
Color is now hsla(0, 50%, 87%, 0.95), not the expected HSLA(0, 50%, 87%, 0.95).

Any change to the color (opacity, color wheel, actual color) also seems to reset the uppercase to lowercase.

larz0 · 2014-12-21T17:10:10Z

This should be a property in brackets.json so that the future inline gradient editor can stick to the preferred case preference; brackets.json will have a UI eventually.

le717 · 2014-12-22T02:03:41Z

@larz0 I haven't touched the preferences system before, that will be new to me. IIRC, I did a toggle button because it was the easiest for me to test with at the time. I will switch it to a preference. Ideas on the name?

@Mark-Simulacrum That's the bug I forgot to document. :oops: That bug will still be valid after I switch to a preference as well.

Mark-Simulacrum · 2014-12-22T03:16:19Z

@le717 My suggestion for the preference is useUppercaseColors if it only applies to colors, but I think it will apply to linear gradients and such in the future, which impedes this naming. Perhaps quick-edit.useUppercase is better, but not sure. Just a few thoughts.

le717 · 2014-12-22T04:04:44Z

src/extensions/default/InlineColorEditor/ColorEditor.js

@@ -64,6 +67,7 @@ define(function (require, exports, module) {
        this._color = tinycolor(color);
        this._originalColor = color;
        this._redoColor = null;
+        this._isUpperCase = PreferencesManager.get("uppercaseColors");


I should rename this as originally it represented if the text was currently uppercase or not.

You need to listen for this preference changing as follows:

PreferencesManager.on("change", "uppercaseColors", function () { this._isUpperCase = PreferencesManager.get("uppercaseColors"); }.bind(this));

Hm, I have been successfully testing the pref change without reloading or listening with no issues. Change made.

The snippet is causing this._isUpperCase to return undefined no matter the preference. Retrieving the preference directly works every time. 😕

Sorry, I wasn't clear. I think it needs to be done both ways.

This initializes setting when inline editor is opened:

this._isUpperCase = PreferencesManager.get("uppercaseColors");

As long as you close inline editor, change preference, and re-open inline editor, then everything should work correctly.

The following block of code handles case where preference is changed while inline editor is open.

PreferencesManager.on("change", "uppercaseColors", function () { this._isUpperCase = PreferencesManager.get("uppercaseColors"); }.bind(this));

@redmunds I think the error is coming in here. Notice how this._color is also in the prototype, but this is not. I have gone back and forth including it there too or not, but I've always hit issues either way. I have a feeling that somehow, the unit tests editor is not picking up the updated this._isUpperCase value, and including it in the prototype as well will fix it. My question then is how to ensure listening to the change will properly update the prototype and not the object.

Yea, the inline color editor is rather a mess (as already documented)...

@le717 The problem is in the test in this line of code in testConvert():

expect(colorEditor.getColor().toString()).toBe(result);

colorEditor.getColor() returns a tinycolor with _originalInput correctly stored as UPPER case. But tinycolor toString() seems to ignore _originalInput, so I think this is a bug in tinycolor.

@le717 As a workaround, I think that line of testConvert() could be changed to:

expect(colorEditor.getColor().getOriginalInput()).toBe(result);

tinycolor.toString() actually works as the author intended. It does not ignore the original case, but rather normalizes (to lowercase) it before returning it. There was actually no way to get the original input (it wasn't even stored) until I suggested it and sent a patch in the form of tinycolor.getOriginalInput().

Yes, but since you added tinycolor.getOriginalInput(), I think it changes expectations. I would expect that method to be called something like toNormalizedString(). I'll leave it up to you if you want to pursue that.

le717 · 2014-12-22T04:54:31Z

@redmunds The current code should be ready for an initial pass now.

@Mark-Simulacrum quick-edit.useUppercase would work, except there is CSS quick editor, color quick edit, easing curve quick edit, and soon linear-gradient quick edit, so it does not convey it applies only to the color-related quick editors.

busykai · 2014-12-22T13:16:58Z

@le717, there are JSHint errors in tests (xit is not defined).

marcelgerber · 2014-12-22T14:32:18Z

I guess quick-edit.uppercaseColors or something along these lines is fine, as even for an potential gradient quick editor, only the color values would be uppercase. It'd be nice if it had another value for auto-detecting the case used in the editor.

I wonder if we should restrict uppercase language names to Hex colors only, as all the others look quite odd (haven't looked at the code though, so maybe this is already the case).

le717 · 2014-12-22T16:34:07Z

@busykai Fully aware of the JSHint errors. Those are the new tests I initially added. I'll be reusing them once the reworking reaches a more final state. ;)

@marcelgerber quick-edit.uppercaseColors sounds good to me.

It'd be nice if it had another value for auto-detecting the case used in the editor.

Yea, that would be a nice switch, but sounds like another preference-debate akin to
usePreferredOnly.

I wonder if we should restrict uppercase language names to Hex colors only, as all the others look quite odd (haven't looked at the code though, so maybe this is already the case).

I've gone back and forth on that. The feature request asked for making only Hex values upper/lowercase, but somewhere there are probably devs who write the other colors in uppercase too, so why should we restrict them from using their preferred case? Right now, the case preference applies universally.

redmunds · 2014-12-29T21:50:58Z

src/editor/Editor.js

@@ -92,7 +92,8 @@ define(function (require, exports, module) {
        STYLE_ACTIVE_LINE   = "styleActiveLine",
        TAB_SIZE            = "tabSize",
        WORD_WRAP           = "wordWrap",
-        USE_TAB_CHAR        = "useTabChar";
+        USE_TAB_CHAR        = "useTabChar",
+        UPPERCASE_COLORS    = "uppercaseColors";


I prefer "uppercaseColors" for a preference key in case anyone ever wants to use it outside of a quick editor.

List of preferences should be in alphabetic order:

UPPERCASE_COLORS = "uppercaseColors", USE_TAB_CHAR = "useTabChar", WORD_WRAP = "wordWrap";

le717 · 2014-12-30T00:12:12Z

@redmunds Changes pushed. What else do you see?

redmunds · 2014-12-30T01:12:15Z

src/extensions/default/InlineColorEditor/ColorEditor.js

-            colorObject = tinycolor(newColor);
+            var newFormat   = $(event.currentTarget).html().toLowerCase().replace("%", "p"),
+                newColor    = _this.getColor(),
+                isUpperCase = _this._isUpperCase;


Don't really need a new var for _this._isUpperCase, right?

No, it is not. Leftover var from toggle button. Fixed.

redmunds · 2014-12-30T01:16:00Z

Done with initial review.

le717 · 2014-12-30T02:17:24Z

@redmunds Requested changes pushed, still have a comment on the preference listening.

marcelgerber · 2015-01-01T13:26:12Z

src/extensions/default/InlineColorEditor/ColorEditor.js

@@ -217,10 +229,10 @@ define(function (require, exports, module) {
        var handler,
            _this = this;


Just for consistency (I know it's not something you've changed), we usually call this variable self.

I'll change that to self next push.

redmunds · 2015-01-03T18:31:00Z

@le717 This looks good, so you can work on the unit tests now.

le717 · 2015-01-06T20:28:52Z

@redmunds Is there a test suite I can look at for reference? It seems that no matter what I try, I cannot get the test to set uppercaseColors to true.

marcelgerber · 2015-01-06T20:33:17Z

The FileFilters test suite uses some preferences.

redmunds · 2015-01-06T21:38:03Z

@le717 Take a look in Editor-test.js at the "with soft tabs preference off" group of tests.

le717 · 2015-01-07T01:35:28Z

Neither of those are working. :\ Is there a dependency or state that needs to be loaded/enabled first?

redmunds · 2015-01-07T17:23:26Z

src/extensions/default/InlineColorEditor/unittests.js

@@ -473,34 +473,77 @@ define(function (require, exports, module) {
                    expect(colorEditor.getColor().toString()).toBe(result);
                }

-                it("should convert a hex color to rgb when mode button clicked", function () {
+                // Use uppercase colors
+                PreferencesManager.set("uppercaseColors", true);


@le717 Code should only be executed inside it() callback functions to keep each test independent.

Here's what you should do:

Copy entire original describe("conversions", function () { block and give it a unique name such as describe("conversions in UPPER CASE", function () {

At top of describe() block and before first it() call, define these callbacks:

beforeEach(function () { // Enable uppercase colors PreferencesManager.set("uppercaseColors", true); }); afterEach(function () { // Re-disable uppercase colors PreferencesManager.set("uppercaseColors", false); });

This will automatically set preference before each test and cleanup after each test in that describe() block.

Note: the testConvert() function will be defined in both the original and new describe() blocks, so you can move it up 1 level so there's only a single shared copy.

le717 · 2015-01-12T22:58:50Z

@redmunds All changes except mixed-case tests made. However, the uppercase tests are still failing despite uppercaseColors confirmed to be true. I'm thinking it is an implementation error. I'll comment where I think it is coming in, but I am unsure of a fix ATM.

redmunds · 2015-01-13T15:41:10Z

src/extensions/default/InlineColorEditor/unittests.js

+                it("should use uppercase colors", function () {
+                    expect(PreferencesManager.get("uppercaseColors")).toBe(true);
+                });
+                it("should convert a hex color to rgb when mode button clicked", function () {


Each test should have a unique name, so you'll need to add something like "in uppercase" to each of these duplicated tests.

le717 · 2015-01-13T16:49:38Z

@redmunds Test names, mixed case tests, and use getOriginalInput() changes pushed, all tests now pass. I didn't get your comment saying to use getOriginalInput() until later, so when I read your first one my thought was to use that method.

If everything looks good, I'll flatten this.

redmunds · 2015-01-13T18:38:48Z

@le717 Looks good! Squash it.

peterflynn · 2015-01-13T20:44:30Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+        this._isUpperCase = PreferencesManager.get("uppercaseColors");
+        PreferencesManager.on("change", "uppercaseColors", function () {
+            this._isUpperCase = PreferencesManager.get("uppercaseColors");
+        }.bind(this));


Drive-by comment: this leaks listeners. Each time a color editor is opened, another change listener is added to PreferencesManager. The change listener will also keep the entire color editor in memory forever, since it's part of its closure. We shouldn't merge until this is fixed.

Good catch! It sucks that JS objects have a constructor but not a destructor.

@le717 You could add a ColorEditor.destroy() method to remove the listener, which could be called from the InlineColorEditor.onClosed() method.

@redmunds Doing so always throws "undefined is not a function", using both ColorEditor.destroy() and this.colorEditor.destroy() (like surrounding code).

Make sure new method is part of object prototype: ColorEditor.prototype.destroy()

Sorry, I misread that (my thoughts are a bit scrambled right now with classes just starting back). Fixed.

le717 · 2015-01-14T01:57:49Z

@peterflynn @redmunds Listener hopefully patched, hopefully passes your inspection. Already squashed the commits.

redmunds · 2015-01-14T17:38:24Z

src/extensions/default/InlineColorEditor/unittests.js

@@ -503,6 +503,65 @@ define(function (require, exports, module) {
                it("should convert an hsla color to rgba when mode button clicked", function () {
                    testConvert("hsla(152, 12%, 22%, 0.7)", "rgba", "rgba(49, 63, 57, 0.7)");
                });
+                it("should convert a mixed case hsla color to rgba when mode button clicked", function () {


Thanks for the mixed case tests!

redmunds · 2015-01-14T17:45:02Z

Done with review. This looks good. Just one last question about unit tests.

le717 · 2015-01-16T18:00:06Z

Questions answered, switched unit tests to use getOriginalInput(). Ready on your command.

redmunds · 2015-01-16T19:23:37Z

Looks good. Merging.

@le717 Don't forget to add new preference to https://github.com/adobe/brackets/wiki/How-to-Use-Brackets#list-of-supported-preferences Note that prefs are now in alphabetic order.

Use upper/lowercase colors in Inline Color Editor

le717 · 2015-01-16T21:25:06Z

@redmunds Done. I wanted to add that this has the chance of breaking extension if they for whatever reason use the TinyColor version from here (which I don't think they do, they host their own) because of the API changes, so it might be good to list it as a library update in the change log.

marcelgerber · 2015-01-17T23:21:54Z

I just searched through all the extensions on the registry and luckily, there's not a single one using tinycolor. So we're pretty much good to go.

le717 changed the title ~~[REVIEW ONLY] Add toggle case action to inline color editor~~ Add toggle case action to inline color editor Oct 24, 2014

le717 mentioned this pull request Dec 20, 2014

Support rebeccapurple #10250

Closed

redmunds self-assigned this Dec 21, 2014

le717 mentioned this pull request Dec 22, 2014

Support named color rebeccapurple #10252

Closed

le717 reviewed Dec 22, 2014
View reviewed changes

redmunds reviewed Dec 29, 2014
View reviewed changes

redmunds reviewed Dec 30, 2014
View reviewed changes

redmunds added the PR Triage Complete label Dec 30, 2014

marcelgerber reviewed Jan 1, 2015
View reviewed changes

marcelgerber mentioned this pull request Jan 6, 2015

Put an array of all the color names in ColorUtils #10303

Merged

le717 changed the title ~~Add toggle case action to inline color editor~~ Use upper/lowercase colors in Inline Color Editor Jan 6, 2015

redmunds reviewed Jan 7, 2015
View reviewed changes

redmunds reviewed Jan 13, 2015
View reviewed changes

peterflynn reviewed Jan 13, 2015
View reviewed changes

Triangle717 added 2 commits January 13, 2015 18:32

Add upper/lowercase colors option in Inline Color Editor(#9519)

21252c8

Fix leaky listener

90fd4dc

le717 mentioned this pull request Jan 14, 2015

Potential toString vs getOriginalInput confusion bgrins/TinyColor#77

Closed

redmunds reviewed Jan 14, 2015
View reviewed changes

Use getOriginalInput() in tests

c9c7a2e

redmunds added a commit that referenced this pull request Jan 16, 2015

Merge pull request #9596 from le717/issue-9519-take2

e83807d

Use upper/lowercase colors in Inline Color Editor

redmunds merged commit e83807d into adobe:master Jan 16, 2015

le717 deleted the issue-9519-take2 branch January 16, 2015 19:34

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Use upper/lowercase colors in Inline Color Editor brackets-archive/bracketsIssues#8558

Open

		@@ -217,10 +229,10 @@ define(function (require, exports, module) {
		var handler,
		_this = this;

Use upper/lowercase colors in Inline Color Editor #9596

Use upper/lowercase colors in Inline Color Editor #9596

Conversation

le717 commented Oct 19, 2014

Features

Mark-Simulacrum commented Dec 21, 2014

Mark-Simulacrum commented Dec 21, 2014

larz0 commented Dec 21, 2014

le717 commented Dec 22, 2014

Mark-Simulacrum commented Dec 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

le717 commented Dec 22, 2014

busykai commented Dec 22, 2014

marcelgerber commented Dec 22, 2014

le717 commented Dec 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

le717 commented Dec 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Dec 30, 2014

le717 commented Dec 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Jan 3, 2015

le717 commented Jan 6, 2015

marcelgerber commented Jan 6, 2015

redmunds commented Jan 6, 2015

le717 commented Jan 7, 2015

Choose a reason for hiding this comment

le717 commented Jan 12, 2015

Choose a reason for hiding this comment

le717 commented Jan 13, 2015

redmunds commented Jan 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

le717 commented Jan 14, 2015

Choose a reason for hiding this comment

redmunds commented Jan 14, 2015

le717 commented Jan 16, 2015

redmunds commented Jan 16, 2015

le717 commented Jan 16, 2015

marcelgerber commented Jan 17, 2015