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

Codemod for SVG Attributes #6213

Closed
jimfb opened this issue Mar 8, 2016 · 10 comments
Closed

Codemod for SVG Attributes #6213

jimfb opened this issue Mar 8, 2016 · 10 comments

Comments

@jimfb
Copy link
Contributor

jimfb commented Mar 8, 2016

Previously, the whitelist of supported SVG attributes was very incomplete. Rather than constantly playing catch up with the SVG attributes whitelist, we decided it would be better to pass all attributes directly through. For example, the strokeWidth attribute is now deprecated in favor of the stroke-width attribute (the "standard" attribute for SVG elements).

It would be good if we had a codemod to help SVG users migrate to v15. It can rewrite all the known attributes (including className->class for SVG elements).

@gaearon
Copy link
Collaborator

gaearon commented Mar 8, 2016

Relevant React PR: #5714

@sebmarkbage
Copy link
Collaborator

SVG has equivalents for these on the style object. I think that's the direction SVG is intending to go to unify the style models with HTML/CSS.

Shouldn't we recommend that both strokeWidth/stroke-width are both changed to style={{ strokeWidth: ... }}? That way we're back to camelCase again.

@jimfb
Copy link
Contributor Author

jimfb commented Mar 8, 2016

Not for the kebab-case attributes that aren't styles, for instance glyph-name:
https://www.w3.org/TR/SVG11/fonts.html#GlyphElementGlyphNameAttribute

@sebmarkbage
Copy link
Collaborator

Pretty sure SVG fonts isn't supported cross browser anyway. Don't expect that one to be used. Mostly edge cases.

Is there a common one? I think commonality and things that are passed down through spread or forwarding properties (e.g. className) are the most important to get right. An edge case for a little used attribute is better if we can keep the rest consistent.

@jimfb
Copy link
Contributor Author

jimfb commented Mar 8, 2016

I don't know what's commonly used and what isn't; I read the spec to understand, and the spec doesn't mention popularity metrics :). But I disagree with your supposition that it's better to remain consistent with a crappy API than to avoid edge cases (past, present, and future). IMO, It would be better to "fix" the legacy API decisions. Regardless, there are at least 71 SVG attributes with kebab case today, and more may be introduced in the future. I think it's better to remain consistent with the SVG API and pass everything through, rather than introduce special edge cases every time something pops up. Passing everything through is easy to understand, conforms to the spec, and will future proof us against future changes in the spec and an ever-growing whitelist - it is clearly the better route, IMHO.

@sebmarkbage
Copy link
Collaborator

They're not mutually exclusive.

We can still recommend using the style object instead of attributes. That way the recommendation is still camel case.

On Mar 8, 2016, at 8:25 AM, Jim notifications@github.com wrote:

I don't know what's commonly used and what isn't; I read the spec to understand, and the spec doesn't mention popularity metrics :). But I disagree with your supposition that it's better to remain consistent with a crappy API than to avoid edge cases (past, present, and future). It would be better to "fix" the legacy API decisions. Regardless, there are at least 71 SVG attributes with kebab case today, and more may be introduced in the future. I think it's better to remain consistent with the SVG API and pass everything through, rather than introduce special edge cases every time something pops up. Passing everything through is easy to understand, conforms to the spec, and will future proof us against future changes in the spec and an ever-growing whitelist - it is clearly the better route, IMHO.


Reply to this email directly or view it on GitHub.

@sebmarkbage
Copy link
Collaborator

I think that I disagree about className being a crappy API and that it is legacy. The decision to promote properties over attributes is something I very much still thinks was the right call. For example, there is no defaultValue HTML attribute - only a property. We need that. There is some state in SVG nodes as well. We don't make any of them controlled yet but if there is something in there that we need to make controlled, I suspect there will be special cases added to handle that.

Likewise, the style object is also another special case.

The web is going AWAY from the serialized form to prefer rich complex data types. E.g. CSSOM moving to value objects for color, lengths, etc. instead of just strings. By clinging to attributes we're actually encouraging legacy. The only thing I wish we did was call it class instead of className to avoid these discussions/confusions. That would've been another special case too. That doesn't make the rest of the property-forward decisions wrong.

@zpao
Copy link
Member

zpao commented Mar 8, 2016

The point of the codemod would be to get rid of warnings, not necessarily move everything to style, even if that's the "right thing" in the long term. Moving everything to style can be a distinct codemod.

Let's do it and put it in https://github.com/reactjs/react-codemod

@sebmarkbage
Copy link
Collaborator

If you think that's an easier story, sure. We should make a blog post about it at least though.

Not sure about deprecating className though.

@gaearon
Copy link
Collaborator

gaearon commented Mar 15, 2016

Closing as #5714 has been reverted in #6243.

@gaearon gaearon closed this as completed Mar 15, 2016
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

No branches or pull requests

4 participants