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

Applying markup with Text() panics often #199

Open
bzub opened this issue Apr 21, 2018 · 6 comments
Open

Applying markup with Text() panics often #199

bzub opened this issue Apr 21, 2018 · 6 comments
Labels
Milestone

Comments

@bzub
Copy link
Contributor

bzub commented Apr 21, 2018

Using an Applyer that isn't compatable with a Text Node doesn't appear to be handled currently.

Simple example:

package main

import (
	"github.com/gopherjs/vecty"
	"github.com/gopherjs/vecty/elem"
)

type pageView struct {
	vecty.Core
}

func main() {
	vecty.RenderBody(&pageView{})
}

func (c *pageView) Render() vecty.ComponentOrHTML {
	return elem.Body(
		vecty.Text("hello",
			vecty.Markup(
				vecty.Attribute("test", true),
			),
		),
	)
}

Result:

TypeError: obj[$externalize(...)] is undefined

Code:
https://github.com/gopherjs/vecty/blob/1d629507357c2e861fbc2b7cf1553c1cb0789399/dom.go#L1251

@slimsag
Copy link
Member

slimsag commented Apr 22, 2018

Agreed, we should be producing a clean panic here and documenting this or refusing to accept vecty.Attribute here via the type-system instead.

vecty.Text is just a DOM TextNode, essentially, so this is not allowed. The function takes vecty.MarkupOrChild today but in reality you can't give it children (Component, *HTML, List, KeyedList, or nil) I think. You can only provide it with vecty.Markup and even then, I think only vecty.Key is actually valid (and even that may not be, I'm not sure). Not sure how we missed this.

WDYT @pdf ?

@pdf
Copy link
Contributor

pdf commented Apr 22, 2018

I think I'd be fine with just changing the signature to vecty.Text(text string). Though vecty.Key might be valid here, I think it's fine to require users to wrap the text in a span or something if they want efficient re-ordering.

@slimsag
Copy link
Member

slimsag commented Apr 22, 2018

Yeah, or we could always offer an vecty.KeyedText if it is really needed in the future.

👍 to just changing signature to vecty.Text(text string)

@bzub
Copy link
Contributor Author

bzub commented Apr 22, 2018

Sounds good to me as well, to change the signature of Text().

There's still the issue of what to do when Apply(h) is called on a *HTML that's a text node. I make my own types that implement Applyer quite often. There's no way to tell in that situation if a *HTML is a text node (without being hacky). Perhaps we could add a (*HTML).IsText() method?

@slimsag
Copy link
Member

slimsag commented Apr 23, 2018

@bzub Could you elaborate / provide examples of Applyers that you make yourself?

@bzub
Copy link
Contributor Author

bzub commented Apr 23, 2018

I'll have a concrete example that I'm happy with in a few days, hopefully. So I'll elaborate with a simple example for now.

I've created types that satisfy both Component as well as Applyer, with the Component's outer-most-node's markup applied entirely by its corresponding Applyer + user supplied markup. This is to support web libraries that rely heavily on classes/attributes/properties for CSS/JS functionality. The users of my components can either use the components normally as a Component or they can use them as a "smart" Applyer in their own elements/components. The Applyer is smart because it may contain somewhat complex logic based on the values within the component's struct.

Going beyond this, in my experience the web library components I'm trying to implement in Vecty often have child elements that each need special markup applied as well. So my idea was to make each of those building blocks as types that satisfy Component and Applyer as well. That way it's as simple as possible to think about customizing individual pieces of a complex component, while still ensuring each peice gets the markup expected by the web library being used.

Here's an HTML example.

Here's a basic render example. The Applyer portion is missing but could be anything, you can see the component itself is passed to the Markup() function:

func (c *ApplyerComponent) Render() vecty.ComponentOrHTML {
	return elem.Div(
		vecty.Markup(
			c,
			c.UserSuppliedMarkup
		)
		// Children
		&ChildComponent{Label: "Child Component 1"},
		&ChildComponent{Label: "Child Component 2"},
	)
}

@slimsag slimsag added this to the 1.0.0 milestone Jul 1, 2019
@slimsag slimsag added the bug label Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants