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

Doctype preamble for elem.Html #91

Closed
daenney opened this issue Dec 9, 2023 · 4 comments · Fixed by #95
Closed

Doctype preamble for elem.Html #91

daenney opened this issue Dec 9, 2023 · 4 comments · Fixed by #95
Assignees
Labels
enhancement New feature or request

Comments

@daenney
Copy link
Contributor

daenney commented Dec 9, 2023

When using elem.Html, everything gets neatly wrapped in an <html></html> element as expected.

But it's expected/required by browsers to have a <!DOCTYPE html> preamble. It'll work without it, but you immediately get a warning in the console as without the preamble you're rendering in the legacy/quirks mode which is usually not desirable. Since a document can only have a single html element, it seems like elem-go could safely emit it.

So two questions:

  • Have I missed some built-in way of doing this?
  • I'm happy to submit a PR to add this, but it seems the way to do it would be to special case the Html element in RenderTo() which feels a bit iffy. Is there a better way?
@chasefleming
Copy link
Owner

chasefleming commented Dec 10, 2023

@daenney Great catch! That would be great if you could add it. Maybe, we could do something like:

func Html(attrs attrs.Props, children ...Node) *Element {
    return NewElement("html", attrs, children...).withCustomOptions(CustomOptions{OpeningTag: "<!DOCTYPE html>"})
}

I've been avoiding chaining since the pattern isn't used elsewhere in the library, but in this case since it's not an exposed API and it would be used so infrequently, I'm not opposed to it. What do you think? I guess we could also have a new function like NewElementWithCustomRender but that may duplicate more code than we'd like.

@chasefleming
Copy link
Owner

@daenney What I wrote earlier was incorrect since the doctype preamble actually comes before the <html>. Apologies for the oversight. Because <!DOCTYPE html> is so common, I think it's probably fine to just hard code it in the Render method:

func (e *Element) Render() string {
    var builder strings.Builder

    // Automatically add DOCTYPE for 'html' elements
    if e.Tag == "html" {
        builder.WriteString("<!DOCTYPE html>")
    }

    e.RenderTo(&builder)
    return builder.String()
}

If anyone needs different preambles down the road we could have a custom option in Render maybe. I think the simple approach is fine here until additional customization is requested. Do you still want to take this?

@chasefleming chasefleming added the enhancement New feature or request label Dec 15, 2023
@daenney
Copy link
Contributor Author

daenney commented Dec 15, 2023

Ah fair enough. Sorry for not responding to the earlier comment, it completely slipped my mind. I'll take a look at whipping up a PR for this next week.

daenney added a commit to daenney/elem-go that referenced this issue Dec 15, 2023
The doctype preamble needs to be emmitted in order to ensure browsers
don't switch to legacy/quirks mode rendering.

Fixes: chasefleming#91
@chasefleming
Copy link
Owner

@daenney thanks for adding the PR! I created another follow up ticket for this issue as well that would introduce a new RenderWithOptions so people can also disable the preamble still if they need to: #96

Curious what you think. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants