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

Implements custom elements with xml namespaces (for SVG) #622

Closed
wants to merge 1 commit into from

Conversation

oderwat
Copy link
Sponsor Contributor

@oderwat oderwat commented Oct 22, 2021

I added namespaces to elements and added a method to create custom tags. The reason is, that I wrote an auto converter that can translate HTML to go-app declarative syntax. While experimenting with it, I found that SVG support was missing. But adding all of the SVG seems to be a lot of work. After making a simple variant for custom elements I realized that this will not work for SVG because those elements need to be in a special namespace. So I added namespace support.

I think my implementation is quite nice. I added a test and documentation with an example of the usage:

func SVG(w, h int) app.HTMLCustom {
	return app.Custom("svg", false, app.SVG).
		Attr("width", w).Attr("height", h)
}

func Circle(cx, cy, r int, stroke string, width int, fill string) app.HTMLCustom {
	return app.Custom("circle", false, app.SVG).
		Attr("cx", cx).
		Attr("cy", cy).
		Attr("r", r).
		Attr("stroke", stroke).
		Attr("stroke-width", width).
		Attr("fill", fill)
}

func (c *Dynamic) Render() app.UI {
	return SVG(100, 100).Body(
		Circle(50, 50, 40, "black", 3, "red"),
		app.Text("Sorry, your browser does not support inline SVG"),
	)
}

Of course, it adds the "xmlns" field to every element. But I think this is better than having another interface for SVG.

I am contemplating if there should be an "xmlns" method instead which sets the namespace for every element. That may be a better solution than adding the namespace to the Custom() method. Maybe I try that too and push a commit that uses this later on.

EDIT: There is another implementation using an XMLNS() method on the elements instead of the many parameters to Custom(). See below!

@oderwat oderwat changed the title Implements custom tags e.g. app.custom("svg") Implements custom tags e.g. app.Custom("svg") Oct 22, 2021
@oderwat oderwat changed the title Implements custom tags e.g. app.Custom("svg") Implements custom tags and element namespaces Oct 22, 2021
@oderwat oderwat changed the title Implements custom tags and element namespaces Implements custom tags and element namespaces (for SVG) Oct 22, 2021
@oderwat oderwat changed the title Implements custom tags and element namespaces (for SVG) Implements custom elements with xml namespaces (for SVG) Oct 22, 2021
@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Oct 22, 2021

The second commit changes how the xmlns is set by using a special method XMLSN(). It also defaults Custom() to non-self-closing elements and adds a CustomSC() for self-closing elements. I think both changes fit better into the existing code and makes the usage more straightforward:

func SVG(w, h int) app.HTMLCustom {
	return app.Custom("svg").
		XMLNS(app.SVG).
		Attr("width", w).Attr("height", h)
}

func Circle(cx, cy, r int, stroke string, width int, fill string) app.HTMLCustom {
	return app.Custom("circle").
		XMLNS(app.SVG).
		Attr("cx", cx).
		Attr("cy", cy).
		Attr("r", r).
		Attr("stroke", stroke).
		Attr("stroke-width", width).
		Attr("fill", fill)
}

func (c *Dynamic) Render() app.UI {
	return SVG(100, 100).Body(
		Circle(50, 50, 40, "black", 3, "red"),
		app.Text("Sorry, your browser does not support inline SVG"),
	)
}

I would love to see this added as it makes SVG handleable without all the Raw() shenanigans.

Copy link
Owner

@maxence-charriere maxence-charriere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to use the xmlns to know if a NS element should be created. At this point I'm wondering if there should be just an Elem() and SelfClosing() functions.

The reason a did not implement SVG is that it has its own specs and attributes and I did not want to add this to the main package. I'm really wondering if it is worth translating everything. In my projects I had been pretty successful using them in Raw().

)

// HTMLCustom is the interface that describes a custom HTML element including its namespace.
type HTMLCustom interface {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this could be generated since it takes global attrs and global event handlers.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I missed that because my first implementation did not use the "XMLNS" attribute. But this can go. I make the modification.

}

// CustomSC returns an self closing HTML element of the given name in the given namespace
func CustomSC(name string) HTMLCustom {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still returns HTMLCustom which has the Body method. User might make errors with this by setting a body.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I agree that is bad.

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Oct 25, 2021

The reason a did not implement SVG is that it has its own specs and attributes and I did not want to add this to the main package. I'm really wondering if it is worth translating everything. In my projects I had been pretty successful using them in Raw().

I had the "raw" version in my HTML 2 go-app declarative converter but think it would be more useful to have those "custom" ways to create an abstraction. Like with my example. You can build a little library of SVG helpers to create graphs and don't need to go back to string manipulation.

I don't think that the whole SVG spec should be included and if, then in a separate package maybe.

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Oct 25, 2021

I added a new implementation using generate and Elem("custom")

  • I extended generate so it can also handle Elem() (which needs a parameter that leads to a slightly different implementation and test)
  • I still added a test for Elem() (elem_test.go) to make sure it works as expected
  • I did not like to add SelfClosing() as method to HTMLElem when ElemSC() is so much easier to do.

What do you think?

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Oct 25, 2021

I don't understand why it changes scripts.go. Shouldn't the script be the same for all Go installations with the same version? I use v1.17.2 of Go which is the most current one and should have the "best" wasm file with it.

If the file differs between systems or is dependent on Go versions it may be better to have it included in another way maybe?

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Nov 1, 2021

@maxence-charriere I actually need to know if you will take this (or a similar) PR eventually.

@maxence-charriere
Copy link
Owner

I think so, I see the need for a custom element.

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Nov 1, 2021

What did it need to make this go through then?

@maxence-charriere
Copy link
Owner

I need a bit time to think it. I ll probably take your branch and modify it directly.

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Nov 1, 2021

OK!

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Nov 1, 2021

As mentioned I wrote a HTML to declarative converter which helps us to translate sites quickly esp. for prototyping. It may help you to see some example of what the current implementation looks like:

func Calendar() app.UI {
	return app.Section().Body(
	app.Div().Class("mx-auto", "container", "py-20", "px-5").Body(
		app.Div().Class("w-full", "flex", "items-cente", "justify-between").Body(
			app.Div().Body(
				app.A().TabIndex(0).Role("link").Class("cursor-pointer", "focus:text-gray-400", "hover:text-gray-400", "text-xs", "xl:text-base", "text-gray-900", "dark:text-gray-100").Text("September 2018"),
				),
			app.Div().Class("md:flex", "justify-center", "hidden", "w-1/2").Body(
				app.Div().Class("flex", "items-center").Body(
					app.Button().Class("cursor-pointer", "focus:text-gray-400", "hover:text-gray-400").Aria("label", "calendar backward").Body(
						app.Elem("svg").XMLNS("http://www.w3.org/2000/svg").Class("").Attr("width", "12").Attr("height", "12").Attr("viewBox", "0 0 24 24").Attr("stroke-width", "1.5").Attr("stroke", "#353F47").Attr("fill", "none").Attr("stroke-linecap", "round").Attr("stroke-linejoin", "round").Body(
							app.Elem("path").XMLNS("http://www.w3.org/2000/svg").Attr("stroke", "none").Attr("d", "M0 0h24v24H0z").Attr("fill", "none"),
							app.Elem("polyline").XMLNS("http://www.w3.org/2000/svg").Attr("points", "15 6 9 12 15 18"),
							),
						),
					app.Button().Class("cursor-pointer", "ml-20", "focus:text-gray-400", "hover:text-gray-400").Aria("label", "calendar forward").Body(
						app.Elem("svg").XMLNS("http://www.w3.org/2000/svg").Class("").Attr("width", "12").Attr("height", "12").Attr("viewBox", "0 0 24 24").Attr("stroke-width", "1.5").Attr("stroke", "#353F47").Attr("fill", "none").Attr("stroke-linecap", "round").Attr("stroke-linejoin", "round").Body(
							app.Elem("path").XMLNS("http://www.w3.org/2000/svg").Attr("stroke", "none").Attr("d", "M0 0h24v24H0z").Attr("fill", "none"),
							app.Elem("polyline").XMLNS("http://www.w3.org/2000/svg").Attr("points", "9 6 15 12 9 18"),
							),
						),
					),
				),
			app.Div().Body(
				app.A().Class("text-indigo-700", "cursor-pointer", "focus:text-indigo-400", "hover:text-indigo-400").Href("javascript:void(0)").Body(
					app.H4().Class("", "text-xs", "lg:text-base", "font-light", "text-right").Text("Month view"),
					),
				),
			app.Div().Body(
				app.A().Class("cursor-pointer", "text-gray-700", "focus:text-gray-400", "hover:text-gray-400").Href("javascript:void(0)").Body(
					app.H4().Class("text-xs", "lg:text-base", "", "dark:text-gray-200", "font-light", "text-right", "ml-6").Text("Week view"),
					),
				),
			app.Div().Body(
				app.A().Class("cursor-pointer", "text-gray-700", "focus:text-gray-400", "hover:text-gray-400").Href("javascript:void(0)").Body(
					app.H4().Class("text-xs", "lg:text-base", "", "dark:text-gray-200", "font-light", "text-right", "ml-6").Text("Day view"),
					),
				),
			),
....

P.S.: Seeing this I wonder why we create empty Class("") attributes. Maybe I want to clean that up in the converter. It seems like they are in the original HTML. Well fair enough.

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Jan 9, 2022

I just updated this to the latest version of go-app (as we use it quite a lot now in our projects).

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Jan 9, 2022

Looks like I missed something as I got build errors. No time to check this currently. Our apps seem to compile fine.

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Mar 14, 2022

Those test results do not make any sense to me. Do you have an idea what the problem is?

image

For:

image

How can stob() be undefined in line 484?

go test -v -race ./... also runs totally fine on my local system.

@maxence-charriere
Copy link
Owner

maxence-charriere commented Mar 14, 2022

I removed stob. Had some problems with some go versions.

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Mar 14, 2022

I removed stob. Had some problems with some go versions.

I see. I forgot to update my forks master for some mysterious reason ;-)

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Apr 11, 2022

@maxence-charriere What do you think about this PR? Is there anything I can do, to increase your acceptance?

@maxence-charriere
Copy link
Owner

I want to make some work about how the html elements are dealt with. Keep this open since your work as often be very insightful about how the package should evolve.

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Apr 11, 2022

Sadly this PR is holding us back to use your releases for our development as we use app.Elem (mostly for SVG) instead of app.Raw and I actually do not want to rewrite the pages using this.

The other PRs (especially the loading progress) are more optional, even if the SkipAllUpdates seem to have quite some impact. We can gate those more easily in builds and just use them on checkpoint releases.

So if you only think about the implementation itself, but not about the "how to use it". then you may consider accepting the PR and changing the implementation later, maybe together with the introduction of generics.

I think having app.Elem() and the accompanying XMLNS will remove the app.Raw() usage at least for SVG and its sub-elements.

P.S.: I came back to this because pojntfx/html2goapp#1 (comment) mention this same problem and I noticed that you started to add notifications to the master branch which makes some of my later work obsolete in the near future.

- Extended generate so it can also handle Elem()
- Added a test for Elem() (elem_test.go)
- I did not like to add SelfClosing() as method to HTMLElem when
  ElemSC() is so much easier to do.
@oderwat
Copy link
Sponsor Contributor Author

oderwat commented May 25, 2022

We are (extensively) using this in our projects and it proves quite usefull when creating SVG on the fly. So I hope that this PR or something similar will finally be accepted.

@maxence-charriere
Copy link
Owner

maxence-charriere commented May 26, 2022

An update on this. I’m planning changes where this will be possible. Keeping this open since i am using it as a reference for the need.

Working on it now.

@oderwat
Copy link
Sponsor Contributor Author

oderwat commented Jun 25, 2022

Closing because #739 is making the dream true ;-)

@oderwat oderwat closed this Jun 25, 2022
@nathanhack nathanhack mentioned this pull request Sep 25, 2023
Closed
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

Successfully merging this pull request may close these issues.

2 participants