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 support for patternUnits attribute. Fix #1535 #1548

Closed
wants to merge 3 commits into from
Closed

Add support for patternUnits attribute. Fix #1535 #1548

wants to merge 3 commits into from

Conversation

nhunzaker
Copy link
Contributor

This fixes #1535

The patternUnits attribute defines how a pattern's coordinate system is defined (x, y, width, height). This is particularly important when using repeatable patterns in SVG. This commit adds this attribute to the lists of known properties.

I used #1487 as a reference point, please let me know if this needs to be added anywhere else.

result

@@ -108,6 +108,7 @@ var knownTags = {
param: true,
path: true,
pattern: false,
patternUnits: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually a list of tags, not attributes -- so patternUnits shouldn't need to be here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I should not have been such a copy cat. I've addressed this in 81cf316

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@zpao
Copy link
Member

zpao commented May 27, 2014

Couple things:

  1. patternUnits is SVG so should go in https://github.com/facebook/react/blob/master/src/browser/ui/dom/SVGDOMPropertyConfig.js
  2. Since it's camelCased we'll need the extra map in DOMAttributeNames as well.
  3. Please test when you've done that to ensure it works. Run grunt build and get a build of React that you can use (then make sure a change in value gets reflected).

@nhunzaker
Copy link
Contributor Author

Went ahead and rebased master and moved everything over to SVGDOMPropertyConfig.js. I did a basic build on a current repo that uses patternUnits. Things look good, but I'll conduct some more rigorous testing tomorrow morning. JSConf calls :)

@nhunzaker
Copy link
Contributor Author

Made a test to evaluate patternUnits here: http://www.natehunzaker.com/react-pattern-test/

If I were to add a unit test for this, where would be the best place to put it?

nhunzaker added 3 commits June 1, 2014 10:24
The patternUnits attribute defines how a pattern's coordinate system is
defined (x, y, width, height). This is particularly important when using
repeatable patterns in SVG. This commit adds this attribute to the lists
of known properties.
@zpao
Copy link
Member

zpao commented Jun 18, 2014

We don't actually have unit tests for these properties (one day...), so nothing to worry about there. I'm pulling in this and several other SVG related PRs today, so I'll merge this soon.

@zpao
Copy link
Member

zpao commented Jun 20, 2014

Thanks!

@nhunzaker
Copy link
Contributor Author

Sweet! Very cool
On Jun 20, 2014 12:53 PM, "Paul O’Shannessy" notifications@github.com
wrote:

Thanks!


Reply to this email directly or view it on GitHub
#1548 (comment).

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.

<pattern> tag attribute "patternUnits" should be supported
4 participants