Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support modern browser colors: #12

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

boygirl
Copy link

@boygirl boygirl commented Jan 23, 2016

Hi!

I'm using this package via react-art. This PR adds support for modern browser colors directly on the core color module instead of via svg parse. It also adds a fall back color array, so that colors like "foo" will result in something black being rendered instead of an error.

Thanks!

move all the svg named browser colors to core colors, so they can be used
outside of svg parse. Add a fall back color array, so that when colors do not
match by regex, the color defaults to black instead of erroring
@sebmarkbage
Copy link
Owner

These colors are mostly not very useful because designers generally come up with their own combinations and very few people make use of this list. I'm not sure it makes to add the weight of this list to the download for everyone if it is not going to be used a lot.

The preferred solution is to use a converter such as the one in the SVG parser here that converts it to the numeric value if you need it.

What's your use case for using strings?

@boygirl
Copy link
Author

boygirl commented Apr 26, 2016

@sebmarkbage I see your point about the weight trade off of adding a big list of strings. I was consuming your package via react-art and my goal was closer parity with modern browser svg support. I understand the argument for not adding these colors, but I think that support for transparent and none is pretty crucial. And I think a fallback to black for any other non-supported strings is also sensible. Would you like me to remove the additional colors from this PR and leave those fallback behaviors?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants