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

Improve examples, add defer, async, and pre- loading KaTeX #1580

Merged
merged 9 commits into from
Aug 11, 2018
Merged

Improve examples, add defer, async, and pre- loading KaTeX #1580

merged 9 commits into from
Aug 11, 2018

Conversation

ylemkimon
Copy link
Member

@codecov
Copy link

codecov bot commented Aug 9, 2018

Codecov Report

Merging #1580 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1580   +/-   ##
=======================================
  Coverage   84.49%   84.49%           
=======================================
  Files          79       79           
  Lines        4619     4619           
  Branches      807      807           
=======================================
  Hits         3903     3903           
  Misses        619      619           
  Partials       97       97

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b99de9a...6e5e382. Read the comment docs.

@ylemkimon ylemkimon added this to the Version 0.10.0 milestone Aug 9, 2018
docs/api.md Outdated
// '<span class="katex">...</span>'
} catch (e) {
if (e instanceof katex.ParseError) {
// KaTeX can't parse the expression
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worth including an example of how to render the error, e.g.

html = "KaTeX error: " + e.message.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;');

docs/api.md Outdated
} else {
throw e;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd personally rather have this more complex example in the "Handling Errors" section. We could either return this example as it was, or e.g. encourage the use of {throwOnError: false}, which should cover the most common use case -- and then refer to Handling Errors for alternatives. What do you think?

README.md Outdated
} else {
throw e;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep these examples simple in the README, and refer to the API doc for more. I'd use the {throwOnError: false} option in both the examples above. This also quickly demos the ability to pass in a settings object.

```
> If you do not use `defer` attribute, `renderMathInElement` should be called
right before the closing body tag(`</body>`) or when (or after) `DOMContentLoaded`
event is fired on the `document`.
Copy link
Member

Choose a reason for hiding this comment

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

Should be "If you do not use the defer attribute..."

Also I think it might be nice to include an example of attaching to DOMContentLoaded. onload is cool, but it doesn't seem like a great way to write a complex settings object.

```

Then, call the exposed `renderMathInElement` function in a script tag
before the close body tag:
> The loading of scripts are [deferred using `defer` attribute](https://developer.mozilla.org/en/HTML/Element/script#Attributes)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe start with "In the example above," Also add "the" in "deferred using the defer attribute".

@ylemkimon
Copy link
Member Author

@edemaine Please feel free to edit the pull request directly as you deem fit.

@edemaine
Copy link
Member

edemaine commented Aug 10, 2018

OK, I took a stab at revising. For some reason, I'm unable to push to this branch. Could you pull from https://github.com/edemaine/KaTeX/tree/examples? Or help me debug this error message:

ERROR: Permission to ylemkimon/KaTeX.git denied to edemaine.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

(I'm trying git push git@github.com:ylemkimon/KaTeX.git examples:examples which I thought worked before, but has not been working recently.)

I just noticed ylemkimon/KaTeX forked from daniel3735928559/KaTeX at the top of https://github.com/ylemkimon/KaTeX. I probably don't have access because of that...

@ylemkimon
Copy link
Member Author

ylemkimon commented Aug 11, 2018

@edemaine Thank you for reviewing and revising! As a non-native English speaker, I sometimes find it difficult to find the right wording 😄 I've added a few minor changes.

@ylemkimon ylemkimon merged commit ea1a226 into KaTeX:master Aug 11, 2018
@ylemkimon ylemkimon deleted the examples branch August 11, 2018 00:21
@edemaine
Copy link
Member

Happy to help! It's great to see the documentation improving so much.

@kevinbarabash
Copy link
Member

Love all of the documentation! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants