Skip to content

Add all SVG properties #97

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

Closed
wants to merge 1 commit into from
Closed

Add all SVG properties #97

wants to merge 1 commit into from

Conversation

mattiacci
Copy link
Contributor

Adds all missing SVG properties.
Adds/updates validation for several SVG properties.
Adds tests for all added/modified properties.

Fixes #28 (which was originally opened as CSSLint/csslint#283).

@nzakas
Copy link
Contributor

nzakas commented Dec 11, 2013

Thanks for the pull request. This has come up before and the question has always been whether or not all SVG CSS properties are valid in an HTML context. Before merging in such a change, I'd like to understand why all SVG CSS properties should be added vs. just the ones that are known to work with HTML.

@mattiacci
Copy link
Contributor Author

I understand that this tool was originally intended to be used with HTML CSS, but since SVG is a part of the HTML5 spec, HTML5 parsers can (must?) parse inline SVG, and those SVG elements can be styled in the same stylesheet as the HTML elements, I don't understand why parser-lib shouldn't work with SVG properties.

I do see that if you accept SVG properties, attempting to use those properties to style HTML elements wouldn't cause an error, but that seems like a worthwhile trade off to me. I don't think attempting to use fill: gray on a paragraph is that much different than attempting to use border-collapse: collapse on one.

@nzakas
Copy link
Contributor

nzakas commented Dec 12, 2013

The difference is that one will never work, while one will sometimes work.
Arguably, attempting to use "fill" should result in a warning because it
won't work in any browser. This is my main concern - I think the tool is
doing a disservice if it doesn't flag things that definitely won't work,
which is why I've not included all SVG properties to this point.

@mattiacci
Copy link
Contributor Author

Neither border-collapse nor fill will ever work on a paragraph. border-collapse works on a <table>, and fill works on a <circle>.

I just wish I didn't have to stop using parser-lib if I include any SVG styling in my stylesheets.

@nzakas
Copy link
Contributor

nzakas commented Dec 13, 2013

Sorry about that - honestly, I didn't design this parser very well. It's
not as configurable as it should be, and I unfortunately don't have the
time to make it so. In an ideal world, you could turn on HTML and SVG
properties for validation separately.

@mattiacci
Copy link
Contributor Author

Yeah, I understand the time constraint. What do you think about accepting this, then splitting the properties once it's been made configurable?

I had to try. :-) If you're convinced it's a bad idea, I understand and I'll close this.

@nzakas
Copy link
Contributor

nzakas commented Dec 16, 2013

I have no plans to make it configurable (once again, time constraints), so
we'd be left with a situation I'm not happy with.

I'm not convinced it's a bad idea, I just don't have the time to make it be
the way it should be.

@stubbornella
Copy link
Member

I'm closing this. I'd love to support SVG properties, but as @nzakas said, someone would need to do the work to make the parser more configurable.

@ghost
Copy link

ghost commented May 15, 2015

Just want to add a voice of support for this or similar PRs. The linter is flagging the property fill, which is common in CSS when using inline-SVG iconography. Cheers.

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.

Add all SVG properties
3 participants