Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

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

Merged
redmunds merged 2 commits intoadobe:masterfrom
marcelgerber:color-names
Jan 16, 2015
Merged

Put an array of all the color names in ColorUtils#10303
redmunds merged 2 commits intoadobe:masterfrom
marcelgerber:color-names

Conversation

@marcelgerber
Copy link
Copy Markdown
Contributor

So we (and extension developers) can reuse them whereever we want.
The actual regex is unchanged, it's just getting generated now.

@le717
Copy link
Copy Markdown
Contributor

le717 commented Jan 5, 2015

I like this method better than stuffing them in the regex, but I'll need to check what changes I need to make to the Inline Color Editor to use this. 👍

Comment thread src/utils/ColorUtils.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case this is accepted, can you go ahead and add rebeccapurple to this array (#10250)?

@marcelgerber
Copy link
Copy Markdown
Contributor Author

They are still in the regex, so you don't have to change anything.

@redmunds redmunds self-assigned this Jan 6, 2015
@redmunds
Copy link
Copy Markdown
Contributor

redmunds commented Jan 6, 2015

This looks good. I agree that rebeccapurple should be added to the list.

@le717
Copy link
Copy Markdown
Contributor

le717 commented Jan 6, 2015

@redmunds I will close #10252 in favor of this.

@marcelgerber Whoops, missed that. GitHub's long single-line diffs are hard for me to read.

@marcelgerber
Copy link
Copy Markdown
Contributor Author

I think we need #9596 first for rebeccapurple to work properly, right?
I don't know what happens right now if you try to open an color inline editor on rebeccapurple, though.

@marcelgerber
Copy link
Copy Markdown
Contributor Author

Added support for rebeccapurple (and changed comments in order to match the color names included).
Still, the Color Inline Editor doesn't work too well with rebeccapurple (see #10250 (comment)) (Quick View works perfectly fine, btw), but imho we can still merge it as there won't be many out there wanting to edit rebeccapurple anyway, I guess.

Sooner or later, we should, of course, merge #9596 for full support.

@le717
Copy link
Copy Markdown
Contributor

le717 commented Jan 6, 2015

@marcelgerber I am going to try to finish up #9596 today.

@le717
Copy link
Copy Markdown
Contributor

le717 commented Jan 16, 2015

@redmunds While you're online, can you look into merging this too so we can finish up rebeccapurple support?

Comment thread src/utils/ColorUtils.js
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, does not totally need fixing: I'd reword this to

Sorted array of all the color names in the CSS Color Module Level 3 and 4 (http://www.w3.org/TR/css3-color/).

I don't think we need to specifically state what color(s) from CSS4 is supported, as ideally we'd support all of them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Module Level 4 is ready yet, while Level 3 is.
While it's still in development, we should state which names are available.

@redmunds
Copy link
Copy Markdown
Contributor

Looks good. Merging.

redmunds added a commit that referenced this pull request Jan 16, 2015
Put an array of all the color names in ColorUtils
@redmunds redmunds merged commit f9cd4f7 into adobe:master Jan 16, 2015
@marcelgerber marcelgerber deleted the color-names branch January 16, 2015 20:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants