Repository: maragudk/gomponents
Pull request #30 (2020-10-22 22:27:25)

Add br and hr element helpers
I was playing around with gomponents as I like this approach and found that HR and BR tags are not yet supported. As they need to have attributes and there is no difference between attributes and children elements I added them but made them into an extra file. I wonder if attributes and child elements should not be handled as separate parameters.

My tests are testing a bit more than just the plain elements. Not sure if you like that. Having a bigger scope for the tests makes me more confident in having a good test coverage of the actual usage. But then it may fail for code outside of the tested scope. What do you think about that?

Currently, there is also no way to enforce HTML rules like having LI inside UL or OL and other stuff. I think it would be cool if this would be enforced by the type checker or as a "strict" mode at runtime.

But I find it a very nice idea as is already. I dislike adding raw HTML code inside the source and I also dislike mixing HTML (or template) code with dynamic code like if/then/else or loops.

codecov[bot]: # [Codecov](https://codecov.io/gh/maragudk/gomponents/pull/30?src=pr&el=h1) Report
> Merging [#30](https://codecov.io/gh/maragudk/gomponents/pull/30?src=pr&el=desc) into [master](https://codecov.io/gh/maragudk/gomponents/commit/6d2fb0eeb15d6b9774f127517d160137251264a4?el=desc) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/maragudk/gomponents/pull/30/graphs/tree.svg?width=650&height=150&src=pr&token=JYN032TYAC)](https://codecov.io/gh/maragudk/gomponents/pull/30?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##            master       #30   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9        10    +1     
  Lines          116       118    +2     
=========================================
+ Hits           116       118    +2     
```


| [Impacted Files](https://codecov.io/gh/maragudk/gomponents/pull/30?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [el/solid.go](https://codecov.io/gh/maragudk/gomponents/pull/30/diff?src=pr&el=tree#diff-ZWwvc29saWQuZ28=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/maragudk/gomponents/pull/30?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/maragudk/gomponents/pull/30?src=pr&el=footer). Last update [6d2fb0e...aec6b3f](https://codecov.io/gh/maragudk/gomponents/pull/30?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

markuswustenberg: Hey @oderwat, thanks for your contribution! 😊

> I was playing around with gomponents as I like this approach and found that HR and BR tags are not yet supported.

Yeah, I still have a backlog of supported components. I want to implement most of the components at https://developer.mozilla.org/en-US/docs/Web/HTML/Element , at least the ones that are the most used. Remember that you can always just use `g.El` for components that don't exist as helpers.

> As they need to have attributes and there is no difference between attributes and children elements I added them but made them into an extra file. I wonder if attributes and child elements should not be handled as separate parameters.

Yeah, I thought about that as well. The problem is that you can't have multiple variadic parameters in a function, like: `func someElement(attrs ...g.Attr, children ...g.Node)`, and I quite like the simple variadic parameter design.

> My tests are testing a bit more than just the plain elements. Not sure if you like that. Having a bigger scope for the tests makes me more confident in having a good test coverage of the actual usage. But then it may fail for code outside of the tested scope. What do you think about that?

I think that's fine.

> Currently, there is also no way to enforce HTML rules like having LI inside UL or OL and other stuff. I think it would be cool if this would be enforced by the type checker or as a "strict" mode at runtime.

I intentionally have not gone down this path. I have a feeling it's a can of worms, and it would really complicate the code base. If you want this, you could perhaps use an existing checker on the output of `Render`?

> But I find it a very nice idea as is already. I dislike adding raw HTML code inside the source and I also dislike mixing HTML (or template) code with dynamic code like if/then/else or loops.

Thanks. 😊 Same here.
oderwat: Stupid question but are you or me expected to resolve the conversation? We let this open to the reviewer (which is mostly me).
markuswustenberg: > Stupid question but are you or me expected to resolve the conversation? We let this open to the reviewer (which is mostly me).

Not at all stupid. 😊 I would usually say the one doing the fixing, so in this case, you. But it doesn't matter, you can also leave them open. I'm just trying out Github's code review system. :D
oderwat: Wow I hate it when depending branches get out of control. Let me fix that mess.
oderwat: OK. Done.
markuswustenberg: Excellent, thanks again. 🥳

Patch:
diff --git a/el/content.go b/el/content.go
index 5a4bce9..a4bcd61 100644
--- a/el/content.go
+++ b/el/content.go
@@ -23,3 +23,11 @@ func Li(children ...g.Node) g.NodeFunc {
 func P(children ...g.Node) g.NodeFunc {
 	return g.El("p", children...)
 }
+
+func Br(children ...g.Node) g.NodeFunc {
+	return g.El("br", children...)
+}
+
+func Hr(children ...g.Node) g.NodeFunc {
+	return g.El("hr", children...)
+}
diff --git a/examples/simple/simple.go b/examples/simple/simple.go
index 92bc8cc..56747e4 100644
--- a/examples/simple/simple.go
+++ b/examples/simple/simple.go
@@ -44,6 +44,7 @@ func page(props pageProps) g.Node {
 			),
 			el.Body(
 				navbar(navbarProps{path: props.path}),
+				el.Hr(),
 				el.H1(props.title),
 				el.P(g.Text(fmt.Sprintf("Welcome to the page at %v.", props.path))),
 				el.P(g.Text(fmt.Sprintf("Rendered at %v", time.Now()))),


Test patch:
diff --git a/el/content_test.go b/el/content_test.go
index b31ad2d..9cb1d52 100644
--- a/el/content_test.go
+++ b/el/content_test.go
@@ -37,3 +37,15 @@ func TestP(t *testing.T) {
 		assert.Equal(t, `<p>hat</p>`, el.P(g.Text("hat")))
 	})
 }
+
+func TestBr(t *testing.T) {
+	t.Run("returns a br element in context", func(t *testing.T) {
+		assert.Equal(t, `<p>Test<br />Me</p>`, el.P(g.Text("Test"), el.Br(), g.Text("Me")))
+	})
+}
+
+func TestHr(t *testing.T) {
+	t.Run("returns a hr element with class", func(t *testing.T) {
+		assert.Equal(t, `<hr class="test" />`, el.Hr(g.Attr("class", "test")))
+	})
+}