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

introduced RenderWithOptions method, changed RenderTo signature; modi… #97

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

whisk
Copy link
Contributor

@whisk whisk commented Dec 17, 2023

Draft for RenderWithOptions method, refers to #96

@whisk
Copy link
Contributor Author

whisk commented Dec 17, 2023

As I made some changes in styles package to ensure that CssNode implements Node, that led to changes in styles_test.go to remove circular dependencies.
Any ideas on how to improve that?

@chasefleming
Copy link
Owner

The main package should probably be tested independently without the dependency on subpackages -- since they are optional -- so I think TextNode is fine. Could make the argument that the tests in the elem package should also remove the use of attrs subpackage (e.g. use plains string instead of the attrs constants). We should probably add a test for CSSNode then though in the styles package.

elem.go Outdated
@@ -53,16 +53,21 @@ var booleanAttrs = map[string]struct{}{
attrs.Selected: {},
}

type RenderOptions struct {
DisablePreamble bool
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add this comment above this line please? // DisablePreamble disables the doctype preamble for the HTML tag if it exists in the rendering tree.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, since these options actually are present on any element calling Render what do you think about being more specific with a name like DisableHtmlPreamble, DisableDocumentPreamble, or DisableDocPreamble?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DisableHtmlPreamble looks pretty self-explanatory, as we call it for html tag. Also added the comment.

Copy link
Owner

@chasefleming chasefleming left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@chasefleming chasefleming merged commit 4f0bd60 into chasefleming:main Dec 20, 2023
1 check passed
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