Include svg elements in React.Basic.DOM

[kryft]

I wanted to render an svg using purescript-react-basic, and I noticed that the svg elements seemed to be missing from React.Basic.DOM. It looks like codegen only maps over props.elements.html, but the svg elements (apart from the svg tag itself) are under props.elements.svg (according to https://www.npmjs.com/package/react-html-attributes ). I'm only learning purescript and purescript-react-basic, so I could be missing something, but it seems like it would be relatively straightforward to include all the svg elements as well. I could try to make a PR for this if it's as simple as mapping over props.elements.svg as well.

I'm also wondering if Props_svg is correct? https://github.com/lumihq/purescript-react-basic/blob/master/src/React/Basic/DOM/Generated.purs#L1542 Some of the attributes look like they shouldn't be valid for the svg tag itself, but I'm not sure (maybe the spec allows having them even if they don't do anything).

[maddie927]

That'd be great if you want to take a stab at it. If there are enough of them maybe grouping them in React.Basic.DOM.SVG would be a good idea. I can help out if you get stuck with something.

[maddie927]

I'm not sure about Props_svg to be honest. I don't use SVGs directly very often.

[tesujimath]

Coincidentally, I've just added support for global attributes, which is very similar to what you are asking for here. See #83 and the associated PR (about which I expect some discussion!).

(I suspect some changes may be required upstream in react-html-attributes, that looks like a work in progress to me.)

[kryft]

@spicydonuts I guess there are quite a few of them, and in any case grouping them in React.Basic.DOM.SVG seems like a good idea since they're not really HTML elements. I can try to make a PR for this.

@tesujimath Do you mean some changes might be required in react-html-attributes for your PR or this one?

[tesujimath]

@kryft The changes I need (onKeyDown, etc) are there, but not released, so not in the version which is currently pulled in by purescript-react-basic. If you look at the commit history https://github.com/jackyho112/react-html-attributes/commits/master you'll see some fairly recent, important changes, which is what makes me suspect that what you need may not be there yet either (but I didn't check).

[kryft]

@tesujimath Ah, I see. According to the front page documentation that I linked above https://www.npmjs.com/package/react-html-attributes the SVG elements should already be in htmlElementAttributes['elements']['svg'], but I haven't actually verified that everything is there. Here's hoping. :)

[kryft]

@spicydonuts Hmm, although react-html-attributes does contain a list of SVG elements, it doesn't list the supported attributes of individual SVG elements. Somewhat surprisingly it lists the union of all attributes of all SVG elements as the list of attributes for the <svg> HTML tag. Apparently this is intentional:

all SVG element attributes supported by React are under the 'svg' key to avoid too many duplicated values and unnecessary file size

Unfortunately this means that the current SVGProps type contains a bunch of attributes that shouldn't be there.

Based on some googling, React should support all (or at least nearly all) SVG attributes, so https://www.npmjs.com/package/svg-element-attributes could work (with some name mangling - I assume all attributes should be camel case at least). I could also fix the SVGProps type in React.Basic.DOM while I'm at it.

One thing I didn't quite figure out yet is what the analogue of React.Basic.DOM.Internal.SharedProps should be for SVG. The svg-element-attributes package has a short list of global properties, but that doesn't include event handler attributes like onClick, and they should work with SVG elements too. Maybe I'll just write a conservative list that includes the global properties and the event handler attributes.

[maddie927]

Ah, that sounds like a lot of work. Would this SVG module be better in a library of its own?

[kryft]

Whatever you think is best. :) I'm just learning Purescript (in fact rendering SVG in React was almost the first thing I tried in my first project, so I've barely written anything yet!), so it's hard for me to judge where it makes sense to split something off into its own package.

If you do think it makes sense to include the SVG element types in purescript-react-basic, there shouldn't be that much new code; mainly codegen would need one more dependency (svg-element-attributes), and I would need to refactor index.js to do almost-but-not-quite the same thing twice. I would also fix SVGProps in React.Basic.DOM while I'm at it.

[maddie927]

That seems fine I think.. as long as it's a separate module so it doesn't get included in bundles when it's not used (this shouldn't be the case in an ideal world, but mixing in JS tooling with PS frequently means the PS bundler is not an option, and webpack/rollup can't do the dead code removal on their own because PS exports commonjs instead of es-modules 😞 hopefully that changes soon).

[paluh]

I want to thank you for this addition @kryft!

Maybe I'll just write a conservative list that includes the global properties and the event handler attributes.

Do you think it is still a good idea to add event handlers attributes?

[paluh]

@kryft sorry for my previous spam. I had missed SharedSVGProps ;-)

[kryft]

@paluh Sorry I didn't respond sooner - I didn't immediately understand the question, and didn't have time to figure it out, but I'm glad it answered itself. :) Yes, I did add some event handler attributes to SharedSVGProps. I'm not sure that they're entirely correct (does every element really support all of them?), though, and that applies to the element-specific prop types too, but I hope they're at least usable. I only did some basic testing with SVG features that I'm familiar with myself.