-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Color picker should match color name values #3754
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to share the color regex from src/extension/default/QuickView/main.js (line 178) between these 2 extensions if at all possible. This will make it easier to maintain.
It's currently not possible to share from one extension to another, so this regex will need to be moved to a core util.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would extracting the REGEX to language/CSSUtils.js be suitable? I looked over utils/ and couldn't find something appropriate. It's a small change and perhaps it doesn't justify a file in utils/ by itself. Please advise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think having a single REGEX in CSSUtils is a reasonable solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I was thinking of having it in a new file (say ColorUtils.js) and put the new file in utils folder. And the new file then includes tinycolor library with require("thirdparty/tinycolor/tinycolor-min") so that we can dynamically create a regex with all color names already available in that library. Once we're sharing tinycolor library across multiple extensions, then we also need to move it from extension folder (src\extensions\default\InlineColorEditor\thirdparty) to src\thirdparty\tinycolor\ folder.
I've some code that dynamically create a regex with color names from tinycolor library in my branch rlim/improved-color-picker. The branch is pretty out-of-sync; so if you want to take a look, then you can just pull out InlineColorEditor.js. Below is my code from that file. Ideally, we should also combine this regex with the original regex that checks for other color formats.
InlineColorEditor.colorNameArray = $.map(tinycolor.names, function (value, key) {
return "\\b" + key + "\\b";
});
InlineColorEditor.colorNameRegEx = new RegExp(InlineColorEditor.colorNameArray.join("|"), "g");
@oslego Let me know if you want to do all these refactoring and rewriting a new regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @RaymondLim, thanks for the suggestions! I looked over your branch. There's some cool stuff going on there. I'll work to incorporate your changes regarding tinycolor and the dynamic regex if you're ok to review.
The other color matching stuff in your branch is valuable, but I'd rather not tackle it in this PR at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RaymondLim, Is there any particular reason you want to move the tinycolor lib out of InlineColorEditor/thirdparty/? It is used in multiple places, including the unit tests. What's the value of dynamically generating the CSS color names regex from tinycolor, rather than hardcoding it and moving it to a new file called utils/ColorUtils.js? The color names regex doesn't seem likely to change, given it is following the W3C named colors list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of keeping just one copy of the hard-coded color names by using tinycolor library, but I can agree that we can have our own hard-copy if we're going to maintain a new ColorUtils.js file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving just the hardcoded regex to a ColorUtils.js and not touching tinycolor seems to be the safest and least invasive method at the moment. If you have no objections against this, I shall proceed with the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure you don't have duplicate color names; your current regex has some duplicates -- maroon, black and etc.
You should also have checking for word boundary and hyphenated word in the new utils file by moving the code from Quick View extension.
…dule Level 3 for color names
…tching of colors by name
|
Something bad happened with my rebase. Please ignore this. |
|
Messed-up branch history beyond recognition. Closing this PR. Will open a clean one. |
Introduce matching by color name in the InlineColorEditor. Color names to be be matched according to list on [http://www.w3.org/TR/css3-color/](CSS Colors Module Level 3).
Added options to the REGEX since it is the least intrusive change. Matching colors from a separate array might be faster but requires new start / end index method for the color token.