Skip to content
This repository was archived by the owner on Jul 14, 2025. It is now read-only.

Conversation

qnxor
Copy link
Contributor

@qnxor qnxor commented Jun 13, 2022

As seen here: KaTeX/KaTeX#2003 (comment)

I enabled it in the Discourse code too as seen in the commit. This adds support for \tag, \label, \ref, \eqref so equations can be labeled and referenced (references are clickable too). This is another feature that Katex disables by default. Enabling it brings the Katex implementation in DIscourse more inline with Mathjax. My previous PR added support for persistent macros.

Tested and working in latest Discourse stable.

@pmusaraj

@pmusaraj
Copy link
Contributor

Thanks @qnxor, can you fix the linting error please?

@qnxor
Copy link
Contributor Author

qnxor commented Jun 14, 2022

I would but it gives no details, it's from your prettier which just exits with a 1 code giving no error details. Not sure what is it it doesn't like as the code style I used matches the rest. Have a look and let me know.

@pmusaraj
Copy link
Contributor

It's the assets/javascripts/initializers/discourse-math-katex.js file. You need to:

  • run yarn install at the root of the plugin repository
  • run yarn run prettier --write assets/javascripts/initializers/discourse-math-katex.js

@qnxor
Copy link
Contributor Author

qnxor commented Jun 14, 2022

~$ cd discourse-math
~/discourse-math$ yarn install
00h00m00s 0/0: : ERROR: [Errno 2] No such file or directory: 'install'

Never bothered with yarn or prettier in the past myself (edit: I could install only prettier from node manually without yarn?)

@pmusaraj
Copy link
Contributor

Looks like you have a more recent yarn version than what we expect (we use Yarn 1, you probably have version 2+, you can check the exact version via yarn --version.

@qnxor
Copy link
Contributor Author

qnxor commented Jun 14, 2022

$ yarn --version
0.32+git

I'm on Ubuntu 22.04 with standard apt repos (no ppas)

About your suggested change, it should work and it's equivalent, but I like to keep initializer code separated from the iterator, otherwise one would argue you should do away with the decorate() function entirely and bring its body into the katex() caller. With your suggestion, you now have per-elem processing in both decorate() and katex(), which I'd vote against.

I'll install prettier manually to output something your linter won't complain about.

@qnxor
Copy link
Contributor Author

qnxor commented Jun 14, 2022

Fixed into a new commit. Turns out prettier doesn't like single quoted strings. You can run the checks again and approve it.

@qnxor
Copy link
Contributor Author

qnxor commented Jun 14, 2022

About your suggested change, it should work and it's equivalent,

@pmusaraj sorry, your suggestion is actually not equivalent and it won't work since elem is an argument of the each() handler, it doesn't exist in the parent scope. I marked it as resolved by mistake, I'm not sure if your edit got through; it shouldn't since i mad a new commit after it so if you accept the newest commit then it should all be fine.

My code is also (marginally) more efficient as it doesn't create a new object on every call to decorate() like the original code was doing.

@pmusaraj
Copy link
Contributor

Thanks @qnxor, merging.

@pmusaraj pmusaraj merged commit dbd7c1c into discourse:main Jun 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants