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

Add 'patternContentUnits' to SVG attributes. #1702

Closed
wants to merge 1 commit into from
Closed

Add 'patternContentUnits' to SVG attributes. #1702

wants to merge 1 commit into from

Conversation

nullobject
Copy link

The patternContentUnits attribute is commonly used when defining a SVG pattern.

This work is part of #1657.

@nullobject nullobject changed the title Add 'patternUnits' to SVG attributes. Add 'patternUnits' and 'patternContentUnits' to SVG attributes. Jun 17, 2014
@zpao
Copy link
Member

zpao commented Jun 18, 2014

I'm going to take #1548 which has patternUnits, want to update this to just be patternContentUnits (also not in that other PR that there are 2 places that need to be updated). Feel free to also drop the docs change. We haven't been super consistent there so it's going to need a batch update before next release anyway.

@nullobject
Copy link
Author

@zpao Sure thing, I'll switch it to the patternContentUnits property only.

@nullobject
Copy link
Author

@zpao You mentioned to "also not[e] in that other PR that there are 2 places that need to be updated".

Perhaps my understanding of how DOMAttributeNames is used is totally wrong 😉 but the patternContentUnits attribute is not valid on the <svg> element, so why would I need to define it in the DOMAttributeNames? This could also be incorrect in #1548.

@nullobject nullobject changed the title Add 'patternUnits' and 'patternContentUnits' to SVG attributes. Add 'patternContentUnits' to SVG attributes. Jun 18, 2014
@zpao
Copy link
Member

zpao commented Jun 19, 2014

It's not valid on the <svg> element, but it's valid on <pattern>. SVGDOMPropertyConfig isn't about just the <svg> element but all of them. By default React downcases the string specified in Properties (downcased for initial markup and for most HTML attributes). Case actually matters for some of these, so for those we have the DOMAttributeNames escape hatch which allows you to specify the exact string that is valid.

As an example, <linearGradient gradientTransform="rotate(45)"> works but <linearGradient gradienttransform="rotate(45)"> doesn't. Most (all?) SVG attributes seem to be case sensitive, so while there's a bit of duplication in this file, it's necessary for now.

I strongly encourage you to actually create a build of React with your change and then actually use contentPatternUnits in an SVG to ensure everything works. Until we have automated tests here, that's how we've been testing attributes.

@nullobject
Copy link
Author

@zpao Thanks for the explanation.

I did indeed try my build in Safari, so I assume it mustn't be too fussy about case on the <pattern> attributes. Anyhow, I've updated my PR to include contentPatternUnits in the DOMAttributeNames. Yay!

@zpao zpao closed this in 11486f1 Jun 20, 2014
@zpao
Copy link
Member

zpao commented Jun 20, 2014

Thanks for the quick updates :)

@nullobject
Copy link
Author

@zpao My pleasure, thanks for the awesome framework 😉

@nullobject nullobject deleted the feature/pattern-units branch June 23, 2014 01:07
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.

2 participants