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

<path>{children}</path> not rendered, crashes w/ error #105

Open
cdaringe opened this issue Jan 17, 2018 · 9 comments
Open

<path>{children}</path> not rendered, crashes w/ error #105

cdaringe opened this issue Jan 17, 2018 · 9 comments

Comments

@cdaringe
Copy link

problem

i copied and pasted some of these into my choo app, only to find bel wasn't too happy about the DOM structure.

scripts:browserify.bundle multiple root elements must be wrapped in an enclosing tag

doesn't work:

<svg>
  <path>
    <animateTransform  />
  </path>
</svg>

does work, but non-equivalent:

<svg>
  <path />
</svg>

related: choojs/choo#514

@yoshuawuyts
Copy link
Member

@cdaringe yeah, bel isn't too happy about specific self-closing tags. We recommend to not close tags unless demanded by the spec (which I believe are no tags at all). Bit strange, but usable - I think!

@cdaringe
Copy link
Author

Thx for the follow up. In my example however, there is not a work around. I had to pick a different implementation. Do you concur that first presented example is valid DOM? if so, do you think it should be supported? I'm not advocating strongly one way, just trying to understand the state-of-the-world

@yoshuawuyts
Copy link
Member

Aye, we should probably fix this. Just looked at the spec, and they're using it as a self-closing tag everywhere. We try and follow the DOM spec - so it'd be cool if we could support this for sure!

@yoshuawuyts
Copy link
Member

Aye, we should probably fix this. Just looked at the spec, and they're using it as a self-closing tag everywhere. We try and follow the DOM spec - so it'd be cool if we could support this for sure!

edit: looks like both properties are picked up as self-closing. I'm not sure what's going on then. Would you mind sharing some more information so we can reproduce & fix this error? Thanks!

@cdaringe
Copy link
Author

Of course. If AFK ATM but I'll provide a complete runnable

@cdaringe
Copy link
Author

https://github.com/cdaringe/svg-woes

^ npm start

@yoshuawuyts
Copy link
Member

I think I figured out what's happening!

So <path></path> is considered invalid syntax. Instead bel assumes <path /> to always be self-closing. I suspect our parser was discarding the closing </path>, which means in turn means it was thinking we were exporting two top-level elements. Which is invalid!

I managed to get the example working by removing the trailing </path>, and grouping the two elements in <g> tag.

Hope this fixes your issue! I'm sorry the errors have been confusing; we usually try and provide helpful messages for when things go wrong - but looks like this fel straight through. Sincere apologies!

Example

var LOADER = html`<g>
<path fill="#000" d="M26.013,10.047l1.654-2.866c-2.198-1.272-4.743-2.012-7.466-2.012h0v3.312h0 C22.32,8.481,24.301,9.057,26.013,10.047z" />
<animateTransform attributeType="xml"
  attributeName="transform"
   type="rotate"
   from="0 20 20"
   to="360 20 20"
   dur="0.5s"
   repeatCount="indefinite"/>
 </g>`

@tornqvist
Copy link
Member

We ran into this issue so any times with globalgoals.org where we use the <animate /> tag quite a lot. Actually, looking at the DOM spec for <animate /> it's used within both the path and rect elements. https://www.w3.org/TR/SVG11/animate.html#AnimateElement

@yoshuawuyts your proposed solution works for typical transform animations but for drawing paths or transforming shapes, it has to be inside the actual path. We resolved to injecting the animate tag on load, which works but isn't optimal.

It should be feasible to support both self closing and explicit closing tags in hyperx, right? I've considered giving it a shot a couple of times but the source makes my brain hurt.

@yoshuawuyts
Copy link
Member

@tornqvist yeah, it should definitely be possible - you're right in that it does require a bit of work tho, hey

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

3 participants