Skip to content

Add xmlns to svg attributes. - #6471

Merged
gaearon merged 1 commit into
react:masterfrom
salzhrani:xmlns
Jul 9, 2016
Merged

Add xmlns to svg attributes.#6471
gaearon merged 1 commit into
react:masterfrom
salzhrani:xmlns

Conversation

@salzhrani

Copy link
Copy Markdown
Contributor

No description provided.

@zpao

zpao commented Apr 11, 2016

Copy link
Copy Markdown
Contributor

For some reason I thought we already had this… maybe it was a part of the SVG change that got backed out.

I like this in concept but let's do 2 things

  1. Move this to the HTML config. This is really a general XML thing and applies to XHTML as well.
  2. Remove the test (we don't have tests for any of these normal config attributes)
  3. Maybe make it xmlNS. We've remapped a couple things so they make more sense in JS and this sort of fits the criteria.

@zpao

zpao commented Apr 12, 2016

Copy link
Copy Markdown
Contributor

Actually, I guess just leave it as in the SVG config since there's also xmlns:xlink, which we should get in there too. Can you add that as well?

@elrumordelaluz

Copy link
Copy Markdown

Should we add http://www.w3.org/2000/svg here as the default namespace for SVG? Maybe adding a key?

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@salzhrani updated the pull request.

@salzhrani

Copy link
Copy Markdown
Contributor Author

rebased and squashed.
As for the xmlns case, it seems that capitalization happens to mitigate hyphens and colons. other than that it stays lowercase.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@salzhrani updated the pull request.

@fibo

fibo commented Apr 30, 2016

Copy link
Copy Markdown

As a React newbie, I found weird that React strips off the xmlns attribute while it claims full svg support.
Please add it to enable server side SVG rendering.

@617dev

617dev commented May 3, 2016

Copy link
Copy Markdown

+1 for adding support for this. In the mean time if anyone is interested in a workaround (which I needed to be able to support IE9-11) this is what I came up with:

// explicitly build the SVG to be rendered here so we don't lose the NS
const stringifiedSvg = `<svg xmlns="http://www.w3.org/2000/svg" class="svgClass">
                               <use xlink:href="#linkToSymbolId"></use>
                        <svg/>`

return <div dangerouslySetInnerHTML={{__html: stringifiedSvg}}/>

@fibo

fibo commented May 4, 2016

Copy link
Copy Markdown

I created a package that can add xmlns and doctype, to enable server side SVG rendering: svgx.

@brianreavis brianreavis mentioned this pull request Jul 2, 2016
@kennyjwilli

kennyjwilli commented Jul 6, 2016

Copy link
Copy Markdown

+1 on this. Status update?

@ldct

ldct commented Jul 6, 2016

Copy link
Copy Markdown

Same question here, this is blocking my upgrade to react 15.0. Is anything blocking this PR?

@zpao

zpao commented Jul 6, 2016

Copy link
Copy Markdown
Contributor

Coming back around to look at this again now.

@gaearon

gaearon commented Jul 6, 2016

Copy link
Copy Markdown
Collaborator

There is also #6532.

@zpao

zpao commented Jul 6, 2016

Copy link
Copy Markdown
Contributor

I saw, was going to look into the NS change there. Otherwise I think I like this one better. The capitalization of xml{NS,ns} is annoying, both ways.

@zpao

zpao commented Jul 7, 2016

Copy link
Copy Markdown
Contributor

Ok, going to take this as is (mostly, 1 comment incoming). I like xmlns better than xmlNS, mostly because xmlns:Xlink has to exist.

z: 0,
zoomAndPan: 'zoomAndPan',
xmlns: 0,
xmlnsXlink: 'xml:xlink',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 things:

  1. This value should be 'xmlns:xlink' right?
  2. Can you move these up so they're with the other xml* attributes? Doesn't have to be perfect, we have a few others minorly out of order so we'll do a :sort sometime later.

@salzhrani

Copy link
Copy Markdown
Contributor Author

integrated feedback and squashed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants