#7723 - Adding Support For '0x' Notation Color Format #13154

nyteksf · 2017-03-07T01:44:58Z

Hello. Could somebody please point me in the right direction from here where I am stuck? I have implemented the RegExp used to find "0x..." format colors which are entered as input. I have also added the associated button, and plugged in the related code that it might be used (for the most part). However, I'm now having trouble linking everything up in a non-hackish way (or at all) in large part because tinycolor doesn't recognize 0x notation. As a result I cannot get the color swatches to work, or 0x notation to be easily recognized by the given methods of tinycolor like .getFormat() which is for example used with a switch case in Brackets for control flow. Thanks in advance to anyone who can lend me their support! :)

@zaggino

petetnt · 2017-03-08T21:02:55Z

Hi @nyteksf,

if the issue is that tinycolor doesn't support 0x format, I would suggest that you always convert any potential 0x values to RGB first. Basically kinda like you have at your checkIf0xNotation, but much more simpler. Something like:

function asRGB(color) {
  if (/0x/.test(color)) {
    return color.replace("0x", "#");
  }

  return color;
}

/** somewhere down the line where tinycolor is called **/
tinycolor(asRGB(this._getColor())

// when and only when you need the actual color value (for example presentation) convert it to 0x format: 

if (this._format === "0x") {
  return color.toHexString().replace("#", "0x");
}

return color.toHexString();

zaggino · 2017-03-08T23:47:29Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+        var that = this;
+        //MUST CONVERT 0X TO HEX6 NOTATION AND BACK IN ORDER TO BE ABLE TO USE COLOR W/ TINYCOLOR(COLOR). TINYCOLOR DOESN'T SUPPORT 0x NOTATION!
+        function checkIf0xNotation(color,that){
+            if((/0x/).test(color)){ //Is input in 0x Notation?


should really be /^0x/.test(color)

zaggino · 2017-03-08T23:48:08Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+        function checkIf0xNotation(color,that){
+            if((/0x/).test(color)){ //Is input in 0x Notation?
+                //IF YES, THEN CHANGE/UPDATE SETTINGS:
+                var thatColor = color.replace("0x","#"); //CONVERT 0x TO HEX6, ie 0xFFAACC => #FFAACC


instead of thatColor use something more appropriate, like hexColor

zaggino

doesn't look that bad, as @petetnt said, define helper functions like isColor0xNotation, color0xToHex, colorHexTo0x and use them in your code

ficristo · 2017-03-09T09:26:06Z

@nyteksf if TinyColor doesn't support the 0x notation, I think you should file an issue to https://github.com/bgrins/TinyColor to ask support for it.
(From what I saw newer version of the library still don't support the 0x notation, but you could double check)
ATM that repo doesn't seem very active so maybe nothing will come out anytime soon, but if they aren't aware of the issue it will never be fixed.

…ed error message

zaggino · 2017-03-12T23:17:12Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+
+    function checkSetFormat(color,toStr){
+        if((/^0x/).test(color)){
+            var colorRes = color0xToHex(color,toStr);


do not create a variable if it's used only once, you can do return color0xToHex(color,toStr) directly

zaggino · 2017-03-12T23:18:51Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+        if((/^0x/).test(color)){
+            var colorRes = color0xToHex(color,toStr);
+            return colorRes;
+        }else{


when if branch ends with a return statement, else branch is not required and introduces unnecessary nesting, write

if (condition) { return something; } return somethingelse;

instead of

if (condition) { return something; } else { return somethingelse; }

zaggino · 2017-03-12T23:19:50Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+        hexColor._format = "0x";
+
+        if(toStr !== null){
+            hexColor.toString();


what does this do?
I'd expect toString(); result to be assigned to something?
or did you mean to do return hexColor.toString(); ?

return hexColor.toString() is correct.

zaggino · 2017-03-12T23:22:18Z

src/extensions/default/InlineColorEditor/ColorEditor.js

@@ -328,7 +367,8 @@ define(function (require, exports, module) {
        // TinyColor actually generates to see if it's different. If so, then we assume the color
        // was incomplete to begin with.
        if (newColorOk) {
-            newColorOk = (newColorObj.toString() === this._normalizeColorString(newColor));
+            var colorStr = newColor.replace("0x","#") || newColor;


this newColor.replace("0x","#") || newColor should be wrapped in a function with a proper name like normalizeColorString or something which hints, why we're doing this

zaggino · 2017-03-12T23:22:53Z

src/extensions/default/InlineColorEditor/ColorEditor.js

@@ -363,10 +403,12 @@ define(function (require, exports, module) {

        // Create swatches
        swatches.forEach(function (swatch) {
+            var swatchValue = swatch.value.replace("0x","#") || swatch.value;


again, wrap this in some sort of function

zaggino · 2017-03-12T23:23:30Z

src/extensions/default/QuickView/main.js

+
+                if((/^0x/).test(previewCSS)){
+                   previewCSS = previewCSS.replace("0x","#");
+                   };


this ending bracket } seems to be wrongly indented

zaggino · 2017-03-12T23:26:03Z

@nyteksf added a few comments, please have a look

petetnt

Couple of simple comments, looking good so far! Keep up the good work!

petetnt · 2017-03-12T23:22:01Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+        if((/^0x/).test(color)){
+            var colorRes = color0xToHex(color,toStr);
+            return colorRes;
+        }else{


Nit: space between } else {

petetnt · 2017-03-12T23:22:21Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+    }
+
+    function checkSetFormat(color,toStr){
+        if((/^0x/).test(color)){


Nit: spaces: if ((/^0x/).test(color)) {

petetnt · 2017-03-12T23:23:13Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+     * Convert 0x notation into hex6 format for tinycolor
+     * compatibility: ("0xFFAACC" => "#FFFFFF")
+     */
+    function color0xToHex(color,toStr){


Not a huge fan of the second parameter, maybe rename it to something like convertToString and make it a boolean 🤔

Aha! That's the idea behind what I was going for. Thanks. :)

petetnt · 2017-03-12T23:23:45Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+        var hexColor = tinycolor(format0xToHexColor);
+        hexColor._format = "0x";
+
+        if(toStr !== null){


Nits: Can be written as if (!toStr)

You meant if (toStr) :)

petetnt · 2017-03-12T23:25:28Z

src/extensions/default/QuickView/main.js

-                var previewCSS = gradientMatch.prefix + (gradientMatch.colorValue || match[0]);
+                var previewCSS = gradientMatch.prefix + (gradientMatch.colorValue || match[0]); 
+
+                if((/^0x/).test(previewCSS)){


Nit: spaces if ((/^0x/).test(previewCSS)) {, misaligned bracket on line 407

petetnt · 2017-03-12T23:26:06Z

src/extensions/default/InlineColorEditor/ColorEditor.js

        this._updateColorTypeRadioButtons(colorObject.getFormat());
        this.$colorValue.val(colorValue);
-        this.$currentColor.css("background-color", colorValue);
+        this.$currentColor.css("background-color", checkSetFormat(colorValue,"toStr"));


See the comment about the second paremeter above

nyteksf · 2017-03-13T05:21:05Z

Okay. Firstly, I just want to take a moment to thank you guys, once again, for helping me--each one of these feedback sessions has been worth at least a million articles or book pages by dint of contrast.

That said, in response to my most recent changes, I think I have satisfied your associated change requests. Or no? Please let me know if otherwise, and again, I will hop to that so I can get onto the next one. Good times. :)

zaggino · 2017-03-13T05:58:15Z

src/extensions/default/QuickView/main.js

-                var previewCSS = gradientMatch.prefix + (gradientMatch.colorValue || match[0]);
+                var previewCSS = gradientMatch.prefix + (gradientMatch.colorValue || match[0]); 
+
+                if ((/^0x/).test(previewCSS)) {


can this be a function like:

function ensureHexFormat(str) { return (/^0x/).test(str) ? str.replace("0x","#") : str; }

adding constructs like these without explanation makes the code less readable to me

zaggino · 2017-03-13T05:59:39Z

src/extensions/default/InlineColorEditor/ColorEditor.js

@@ -302,6 +339,10 @@ define(function (require, exports, module) {
    ColorEditor.prototype._normalizeColorString = function (color) {
        var normalizedColor = color;

+        // Convert from 0x notation into hex format string
+        if (color.match(/^0x[0-9a-fA-F]{6}/)) {


you can re-use the ensureHexFormat function I mentioned in the comment before here?

zaggino · 2017-03-13T06:00:48Z

Just two of my comments, lets wait for @petetnt to re-check this too.

…ternalized code fragment

petetnt

Couple of small nits from me + @zaggino's comments, otherwise LGTM. We need to add some tests for this in another commit (and check that the relevant tests still pass)

petetnt · 2017-03-13T07:02:02Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+        var hexColor = tinycolor(format0xToHexColor);
+        hexColor._format = "0x";
+
+        if(convertToStr){


One more nit: if (convertToString) { (we really should enable http://eslint.org/docs/rules/space-after-keywords and similar in our config, our spaces are wildly inconsistent across the codebase 🤔 )

@petetnt #13177

petetnt · 2017-03-13T07:02:31Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+     * Convert 0x notation into hex6 format for tinycolor
+     * compatibility: ("0xFFAACC" => "#FFFFFF")
+     */
+    function color0xToHex(color,convertToStr){


Nit: space after comma color, convertToStr

petetnt · 2017-03-13T07:02:58Z

src/extensions/default/InlineColorEditor/ColorEditor.js

+        return color.toHexString().replace("#","0x");
+    }
+
+    function checkSetFormat(color,convertToStr){


Nit: space after comma color, convertToStr

nyteksf · 2017-03-13T11:56:48Z

Okay. The latest change requests have been fulfilled, or so I believe. As always--please don't hesitate to let me know if there're any more in mind. Otherwise, thanks again. And see you again soon. :)

@zaggino @petetnt

#7723 - Removed stray comment, restored proper indentation of function

zaggino · 2017-03-13T23:26:46Z

src/extensions/default/QuickView/main.js

@@ -325,6 +325,11 @@ define(function (require, exports, module) {
        function hasLengthInPixels(args) {
            return (args.length > 1 && args[1].indexOf("px") > 0);
        }
+
+        // Ensures that input is in usable hex format
+            function ensureHexFormat(str) {


@nyteksf
wrong indentation, do you have brackets-eslint extension installed? it should have given you a warning here

Whoops. That said, it passed my tests. And I had thought I had brackets-eslint installed, but I'm not sure anymore. Is that not the same as what would be found when running the command grunt eslint? I ask as it seems npm just downloaded something. If not, then it should be taken care of from here on out, at least.

zaggino · 2017-03-13T23:29:03Z

Looks good now except one place where the code is incorrectly indented, it'd be a good thing to install brackets-eslint extension and fix the warnings in the code you've modified (you don't need to fix warnings in code you didn't touch of course, we know there're some, but we're trying to get the codebase to be more consistent)

petetnt

I added some missing JSDoc strings, otherwise LGTM if tests pass.

nyteksf · 2017-03-14T11:26:15Z

:)
Thanks!
@petetnt

zaggino · 2017-03-14T11:53:35Z

Merged, thanks @nyteksf

petetnt · 2017-03-14T11:54:41Z

Great job @nyteksf! Thanks for this contribution!

nyteksf added 5 commits March 5, 2017 15:15

#7723 - Beginning to put together patch

508e38f

#7723 - Removed my test CSS document from build

84f7863

#7723 - Added missing semicolons

bd18fc1

#7723 - Corrected comment

05ee335

#7723 - Corrected comment

cf3582a

zaggino reviewed Mar 8, 2017

View reviewed changes

zaggino suggested changes Mar 8, 2017

View reviewed changes

nyteksf added 3 commits March 11, 2017 15:26

#7723 - Finalized implementation of conversion functions, spell check…

c7471d6

…ed error message

#7723 - Cleaned up code as best I could

fa0bb46

#7723 - Restored original whitespace

f8cd911

zaggino reviewed Mar 12, 2017

View reviewed changes

petetnt suggested changes Mar 12, 2017

View reviewed changes

#7723 - Completed change requests of @zaggino and @petetnt

9c7187a

zaggino reviewed Mar 13, 2017

View reviewed changes

#7723 - Modified function '_normalizeColorString' to include prior ex…

c08ceb3

…ternalized code fragment

petetnt reviewed Mar 13, 2017

View reviewed changes

#7723 - Another round of change requests handled

738a6d9

nyteksf and others added 2 commits March 13, 2017 05:54

#7723 - Fixed nits & added ensureHexFormat()

fd6e7d8

Update ColorEditor.js

e71807c

#7723 - Removed stray comment, restored proper indentation of function

ficristo mentioned this pull request Mar 13, 2017

Support for color definition format beginning with "0x" #7723

Closed

nyteksf added 2 commits March 13, 2017 12:22

#7723 - Restored explanatory comment back to feature

e67b898

#7723 - Unnecessary comment removed

1bacc51

zaggino reviewed Mar 13, 2017

View reviewed changes

petetnt approved these changes Mar 14, 2017

View reviewed changes

zaggino approved these changes Mar 14, 2017

View reviewed changes

zaggino merged commit 7372631 into adobe:master Mar 14, 2017

nyteksf and others added 2 commits March 14, 2017 05:09

#7723 - Fixed indentation error

e83621f

Add JSDoc for new functions

1dccc61

saurabh95 added this to the Release 1.9 milestone Mar 14, 2017

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

[CLOSED] #7723 - Adding Support For '0x' Notation Color Format brackets-archive/bracketsIssues#11157

Open

#7723 - Adding Support For '0x' Notation Color Format #13154

#7723 - Adding Support For '0x' Notation Color Format #13154

Conversation

nyteksf commented Mar 7, 2017 • edited Loading

petetnt commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaggino left a comment

Choose a reason for hiding this comment

ficristo commented Mar 9, 2017

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

zaggino commented Mar 12, 2017

petetnt left a 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

nyteksf commented Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaggino commented Mar 13, 2017

petetnt left a 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

nyteksf commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaggino commented Mar 13, 2017

petetnt left a comment

Choose a reason for hiding this comment

nyteksf commented Mar 14, 2017 • edited Loading

zaggino commented Mar 14, 2017

petetnt commented Mar 14, 2017

nyteksf commented Mar 7, 2017 •

edited

Loading

nyteksf commented Mar 13, 2017 •

edited

Loading

nyteksf commented Mar 14, 2017 •

edited

Loading