Skip to content

Implement <svg> support#272

Open
efoken wants to merge 2 commits into
react:mainfrom
efoken:svg
Open

Implement <svg> support#272
efoken wants to merge 2 commits into
react:mainfrom
efoken:svg

Conversation

@efoken

@efoken efoken commented Feb 19, 2025

Copy link
Copy Markdown

#4

@efoken

efoken commented Feb 19, 2025

Copy link
Copy Markdown
Author

Let me know what you think @necolas

There are still some $FlowFixMe that I added inside createStrictDOMSvgComponent which can be fixed by changing the NativeProps type, but I was not sure if that's the best place, or if it may be better to have a bit more differentiation between React Native Props and React Native SVG Props.

Also I am willing to add some tests and probably some examples in the Expo examples app.

@necolas necolas left a comment

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.

Thanks for sharing this quickly and early.

My initial thoughts are that the HTML and SVG interfaces are too intermingled in the code. Although html.svg seemed to make sense at first, I have doubts after looking through this patch and doing a bit more research.

The <svg> element is type SVGSVGElement. So writing <svg.svg> in RSD would be true to spec. And eventually there might be <html.html> as well.

If we were to put everything SVG (inc the <svg> element) under the svg export, it would help keep a clearer separation between HTML and SVG types and logic. DCE has a better chance of removing all the SVG code too.

So in general, having separate Svg functions and checks seems like the better way forward to me. And we can refactor/rename any code that needs to be shared as and where it's needed.

Comment thread packages/react-strict-dom/src/native/modules/createStrictDOMSvgComponent.js Outdated
Comment thread packages/react-strict-dom/src/native/svg.js Outdated
Comment thread packages/react-strict-dom/src/shared/isPropAllowed.js Outdated
Comment thread tools/flow-typed/npm/react-native-svg_vx.x.x.js
Comment thread packages/react-strict-dom/src/dom/html.js Outdated
Comment thread packages/react-strict-dom/src/dom/svg.js Outdated
Comment thread packages/react-strict-dom/src/types/StrictReactDOMSvgProps.js Outdated
@nandorojo

nandorojo commented Feb 26, 2025

Copy link
Copy Markdown
Contributor

This PR is great timing! I just happened to do something similar in my app via a patch and was playing with it before sending a PR. Thanks moving so quickly on it.

@MoOx

MoOx commented May 16, 2025

Copy link
Copy Markdown
Contributor

Is something causing any trouble that we could fix to move forward with this ?

@MoOx

MoOx commented May 13, 2026

Copy link
Copy Markdown
Contributor

@efoken I just saw that you published https://github.com/efoken/react-strict-dom-svg @martinbooth do you think this could be merged into RSD repo ?

@efoken efoken force-pushed the svg branch 4 times, most recently from 452a092 to 52dd494 Compare May 20, 2026 06:01
@efoken

efoken commented May 20, 2026

Copy link
Copy Markdown
Author

@MoOx @martinbooth I updated my PR, ist there anything we can do to get this merged?

@MoOx

MoOx commented May 22, 2026

Copy link
Copy Markdown
Contributor

You might want to read from here #4 (comment)
and also this #479

@efoken

efoken commented Jun 5, 2026

Copy link
Copy Markdown
Author

@MoOx I moved everything to a react-strict-svg package, please have a look. I can also remove my published react-strict-dom-svg package after this is merged and published.

@MoOx

MoOx commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Sadly I am no decider here. poke @martinbooth or fb team

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.

5 participants