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

node() does not support namespacing for SVG elements #49

Closed
gordonbrander opened this issue Jul 20, 2016 · 4 comments
Closed

node() does not support namespacing for SVG elements #49

gordonbrander opened this issue Jul 20, 2016 · 4 comments

Comments

@gordonbrander
Copy link

The node function interface used by Reflex and Reflex drivers does not allow setting a namespace. virtual-dom and our VirtualNode class both support namespaces via a namespace property. (I don't really care about namespaces from an aesthetic standpoint, but they are required for svg elements to work).

@Gozala
Copy link
Contributor

Gozala commented Jul 20, 2016

The reason node did not had support for namespace is due to the fact that react does not have a support for them. That being said React does support some SVG elements so we could have something like svg.js similar to current html.js.

We also could add namespace parameter to node function so that it could be used with VirtualDOM driver & be ignored with React driver.

@gordonbrander Do you have any interest in writing a patch for this ?

@gordonbrander
Copy link
Author

gordonbrander commented Jul 21, 2016 via email

@Gozala
Copy link
Contributor

Gozala commented Jul 25, 2016

Happy to write a patch. I lean toward having an svg() function, so you don't have to set the namespace manually.

You would have to pass namespace somewhere as that's what VirtualDOM implementation would expect.

Either that, or in node(), check for svg keys like "svg", "circle" and set namespace "magically" for these.

This seems like an overkill to me.

Do you have a preference on direction?

Here is what I would suggest:

VirtualNode already has a namespace: ?string we just do not expose a way to pass it in. So I think logical step would be to allow passing namespace in. There are two ways to go about it:

  1. We can introduce breaking change to add modify current node API from:

    type node =
      (tagName:string, properties:?PropertyDictionary, children:?Array<DOM>) =>
      VirtualNode | LazyTree<VirtualNode>

    as follows:

    type node =
      (tagName:string, namespace:?string, properties:?PropertyDictionary, children:?Array<DOM>) =>
      VirtualNode | LazyTree<VirtualNode>
  2. We can introduce new functions like nodeNS which will have API as proposed above:

    type nodeNS =
      (tagName:string, namespace:?string, properties:?PropertyDictionary, children:?Array<DOM>) =>
      VirtualNode | LazyTree<VirtualNode>

One way or another we would also have to do following:

  1. Provide higher level API in form of svg.js similar to this html.js that would export shortcuts for react supported svg elements. Note that react will only allow SVG elements from that list regardless of what we do with namespaces.
  2. Update both drivers reflex-virtual-dom-driver and reflex-react-driver, in both cases just to pass a namespace argument to the VirtualNode constructor. Please note that react will actually ignore the namespace field but that's ok, because as long as tag name will be from the supported svg elements list it will work as expected.

I personally lean towards first option because:


  1. I treat node as a lower lever API anyhow targeted a driver implementations and not a reflex users, they should use html.div or svg.circle anyhow.
  2. Second option just duplicates node API with extra argument & adds extra thing drivers need to implement just to avoid API change.

I might be missing something and can be convinced to go a different route.

@Gozala
Copy link
Contributor

Gozala commented Jul 3, 2017

Latest release introduced elementNS function to create non-html namespaced elements.

@Gozala Gozala closed this as completed Jul 3, 2017
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

No branches or pull requests

2 participants