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

Additional SVG tag and attribute names #1032

Merged
merged 2 commits into from
May 5, 2014
Merged

Additional SVG tag and attribute names #1032

merged 2 commits into from
May 5, 2014

Conversation

fforw
Copy link
Contributor

@fforw fforw commented Feb 6, 2014

The current set of SVG attributes / tag I felt like I needed
them absolutely for my React/SVG editor app.

@plievone
Copy link
Contributor

plievone commented Feb 6, 2014

Related #938. SVG tag additions touch quite many files, see for example diff #868

text: false
text: false,
tspan: false

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@zpao
Copy link
Member

zpao commented Feb 14, 2014

As @plievone mentioned, there are several more places to make changes to before this can be accepted. #868 is a good model.

@zpao
Copy link
Member

zpao commented Apr 11, 2014

Sorry for the delay! I just noticed that you had pushed new commits. Can you rebase though, we've already added dx and dy. We also need a change to the transform to make sure <tspan> gets transformed to React.DOM.tspan. You can do that here: https://github.com/facebook/react/blob/master/vendor/fbtransform/transforms/xjs.js

@fforw
Copy link
Contributor Author

fforw commented Apr 13, 2014

Like this?

@@ -155,6 +156,7 @@ var DefaultDOMPropertyConfig = {
stopColor: MUST_USE_ATTRIBUTE,
stopOpacity: MUST_USE_ATTRIBUTE,
stroke: MUST_USE_ATTRIBUTE,
strokeDasharray: MUST_USE_ATTRIBUTE,
Copy link
Contributor

Choose a reason for hiding this comment

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

You have a double space here.

@fforw
Copy link
Contributor Author

fforw commented Apr 22, 2014

Was red before due to bad rebase timing before.. caught a stray error off master.

This is just rebased again and check out fine in Travis.

@zpao
Copy link
Member

zpao commented Apr 24, 2014

This looks great, thanks a lot! I just split HTML and SVG properties into separate files so unfortunately you have a little bit of work to rebase there, sorry about that timing. But I made the internal diff, so as soon as you're ready, we should be good to go.

fforw added 2 commits April 24, 2014 19:12
The current set of SVG attributes / tag I felt like I needed
them absolutely for my React/SVG editor app.
@fforw
Copy link
Contributor Author

fforw commented Apr 24, 2014

rebased it.

@jonase
Copy link
Contributor

jonase commented Apr 26, 2014

Have you considered including the opacity attributes (#1171) as part of this pull request.

zpao added a commit that referenced this pull request May 5, 2014
Additional SVG tag and attribute names
@zpao zpao merged commit b48a534 into facebook:master May 5, 2014
@zpao
Copy link
Member

zpao commented May 5, 2014

Thanks! Sorry that took so long :/

@sophiebits sophiebits mentioned this pull request Aug 19, 2014
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.

4 participants