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.

Sign up for GitHub

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

Jump to bottom

Add KaTeX rendering engine #14

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Add KaTeX rendering engine #14

wants to merge 15 commits into from

Conversation

ongchi
Copy link
Collaborator

@ongchi ongchi commented Jul 1, 2024 •
edited
Loading

This PR include some breaking changes:

  • Rename the project into wagtail_polymath
  • Add support of KaTex rendering engine
  • Add Django settings options:
    • WAGTAIL_POLYMATH: The default value of rendering engine is mathjax. katex is also supported. Can also specify specific version of rendering engine from CDN, or from self provisioned django static files.
  • Rename html template namespace from wagtailmath to wagtail_polymath

@zerolab
Copy link
Contributor

zerolab commented Jul 4, 2024

Thank you for this @ongchi
Somehow I did not get any notice of the PR. Will aim to look at it soon.
If we're updating the namespace, I think we should:

  1. bump the version to 2.0
  2. add upgrade instructions
  3. should claim wagtail-polymath on PyPI

@zerolab
Copy link
Contributor

zerolab commented Jul 8, 2024

The main changes needed

  1. a rebase on main
  2. keeping the src/ structure

@zerolab zerolab added this to the Version 2.0 milestone Jul 8, 2024
@ongchi
Copy link
Collaborator Author

ongchi commented Jul 21, 2024

Hi @zerolab,
Sorry for the late response, I'm still working on this PR.
This PR would break some APIs as introduces a new rendering engine and bumps the version number anyway. So it could also be a opportunity to rename the project at the same time.

I should find some time to update the README and add some relevant tests in this PR.

@zerolab zerolab mentioned this pull request Jul 22, 2024
Closed
@ongchi ongchi marked this pull request as ready for review September 22, 2024 09:21
@ongchi ongchi requested a review from zerolab September 22, 2024 09:21
zerolab
zerolab requested changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Did a first pass review. Need to check the widget side of things and test it locally in another pass.

Left some minor suggestions/notes

MANIFEST.in Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/wagtail_polymath/config.py
@@ -0,0 +1,58 @@
from django.conf import settings
Copy link
Contributor

Choose a reason for hiding this comment

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

note to self: move settings to a lazy object a la Django settings

src/wagtail_polymath/config.py Outdated
paths = WAGTAILPOLYMATH_SETTINGS.get(key, [])

# Return absolute path to the asset if it's a static file path.
return [versioned_static(path) if finders.find(path) else path for path in paths]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we skip running finders.find if path starts with http ?

ongchi
ongchi commented Oct 5, 2024
View reviewed changes
src/wagtail_polymath/config.py Outdated
@@ -54,5 +55,25 @@
def get_polymath_config(key):
paths = POLYMATH_SETTINGS.get(key, [])

# Return absolute path to the asset if it's a static file path.
return [versioned_static(path) if finders.find(path) else path for path in paths]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it would be good to have this URL validation, so I am adding validation logic for a generic URL.

@ongchi ongchi requested a review from zerolab October 5, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zerolab zerolab Awaiting requested review from zerolab

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Version 2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ongchi @zerolab