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

Add buildHTMLTree #1022

Merged
merged 2 commits into from Dec 18, 2017
Merged

Add buildHTMLTree #1022

merged 2 commits into from Dec 18, 2017

Conversation

edemaine
Copy link
Member

This is a followup to #1017 (Exposing the build tree). It includes a new generateBuildHTMLTree function that skips the MathML step, and exposes that function as __getBuildHTMLTree. See #800 for discussion.

I'd like to have a discussion about the naming of these functions. I find the naming of these functions, and the generateBuildTree and __getBuildTree that come from #1017, to be "incorrect". I don't think these were ever supposed to be called "build trees". "Build" in buildTree was meant as a verb, as in "build a tree". I think it would make more sense to rename buildTree to domTree or something. Alternatively, we could rename generateBuildTree to parseAndBuildTree or something like this, which is what it's doing. I also am not a fan of the get in __get. If there's consensus, I can rename in this PR.

Copy link
Member

@ry-randall ry-randall left a comment

Choose a reason for hiding this comment

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

I have no problem with changing the export names. Hopefully no one has used in the few days it's been available. Would we still consider a breaking change?

): domTree.span {
const options = optionsFromSettings(settings);
const htmlNode = buildHTML(tree, options);
const katexNode = buildCommon.makeSpan(["katex"], [htmlNode]);
Copy link
Member

Choose a reason for hiding this comment

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

Is the katexNode necessary here? Why not just return htmlNode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good question. htmlNode would be natural too. My reasoning here was that there are various CSS rules that only apply with the .katex-display .katex nesting...

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay.

@edemaine
Copy link
Member Author

@rrandallcainc I think because we haven't released a version with this change (right?) it's probably OK, though obviously not ideal.

@ry-randall
Copy link
Member

Yup, agreed that this should not be an issue. Forgot it hasn't been released.

@ry-randall
Copy link
Member

@edemaine These changes look fine to me. What were you thinking for the new naming convention?

@edemaine
Copy link
Member Author

How about the following?

  • __getBuildTree__renderToDomTree (by analogy to renderToString)
  • generateBuildTreerenderToDomTree
  • __getBuildHTMLTree__renderToHTMLTree
  • generateBuildHTMLTreerenderToHTMLTree

Hmm, maybe we should have renderToMathMLTree and/or renderToMathML. I guess we can wait for someone to request that feature.

@ry-randall
Copy link
Member

Using the existing "render" nomenclature sounds good. I agree that consistency would be nice here.

@edemaine
Copy link
Member Author

OK, I did the renames. I also added some clarifying documentation so it's clear what the different functions provide.

It occurs to me that a "no MathML" option would still be neat in that it would allow renderToString to also render HTML only. But maybe it's best not to encourage that behavior... One can achieve it now via __renderToHTMLTree(...).toMarkUp().

Copy link
Member

@ry-randall ry-randall 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. The comments around the exports are a nice addition.

@ry-randall ry-randall merged commit 2d439f0 into KaTeX:master Dec 18, 2017
@kevinbarabash
Copy link
Member

🎉

@edemaine
Copy link
Member Author

@rrandallcainc Thanks for the quick review!

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.

None yet

3 participants