Conversation
|
Let me know what you think @necolas There are still some Also I am willing to add some tests and probably some examples in the Expo examples app. |
There was a problem hiding this comment.
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.
packages/react-strict-dom/src/native/modules/createStrictDOMSvgComponent.js
Show resolved
Hide resolved
| * "circle" | ||
| */ | ||
| export const circle: component( | ||
| ref?: React.RefSetter<HTMLElement>, |
There was a problem hiding this comment.
Need to use the correct ref type for these, e.g., SVGCircleElement
There was a problem hiding this comment.
Yes correct @necolas, but SVG element types do only exist in TypeScript, sadly Flow does not have them ...
| 'alignmentBaseline', // svg | ||
| 'amplitude', // svg | ||
| 'azimuth', // svg | ||
| 'baseFrequency', // svg | ||
| 'baselineShift', // svg | ||
| 'bias', // svg | ||
| 'clipPath', // svg | ||
| 'clipRule', // svg | ||
| 'color', // svg | ||
| 'crossOrigin', // svg | ||
| 'cx', // svg | ||
| 'cy', // svg | ||
| 'd', // svg | ||
| 'diffuseConstant', // svg | ||
| 'divisor', // svg | ||
| 'dx', // svg | ||
| 'dy', // svg |
There was a problem hiding this comment.
We should probably refactor this code and have a separate check for SVG props, as they shouldn't be allowed on the HTML elements.
| */ | ||
|
|
||
| declare module 'react-native-svg' { | ||
| declare module.exports: any; |
There was a problem hiding this comment.
If there are no community Flow types we can use for this package, I can contribute types based on what we use internally
There was a problem hiding this comment.
There are no community types available
| * "circle" | ||
| */ | ||
| export const circle: component( | ||
| ref?: React.RefSetter<HTMLElement>, |
There was a problem hiding this comment.
The ref types need to be updated to their SVG element equivalents
| 'aria-label'?: ?Stringish, | ||
| 'data-testid'?: ?string, |
There was a problem hiding this comment.
These types are already part of StrictReactDOMProps
There was a problem hiding this comment.
Changed it to Pick< StrictReactDOMProps, 'aria-label' | 'data-testid'>, hope that's okay as only these two props are supported on elements, at least not for Native
There was a problem hiding this comment.
Yeah we need to match the web types more closely. Doesn't matter if native doesn't yet have as much support. Same as we do for HTML
|
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. |
|
Is something causing any trouble that we could fix to move forward with this ? |
5 participants
#4