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

Improve katex/math text rendering in ZT #1024

Merged
merged 2 commits into from
Jul 6, 2021
Merged

Improve katex/math text rendering in ZT #1024

merged 2 commits into from
Jul 6, 2021

Conversation

Ezio-Sarthak
Copy link
Member

No description provided.

@zulipbot zulipbot added the size: XS [Automatic label added by zulipbot] label May 8, 2021
@Ezio-Sarthak Ezio-Sarthak changed the title boxes: Avoid rendering duplicate katex markups. boxes: Avoid rendering multiple katex markups. May 8, 2021
@Ezio-Sarthak
Copy link
Member Author

@zulipbot add "area: message rendering"

@Ezio-Sarthak Ezio-Sarthak force-pushed the latex-rendering-fix branch from 09e191c to cc69d4d Compare May 8, 2021 13:26
@zulipbot zulipbot added size: S [Automatic label added by zulipbot] and removed size: XS [Automatic label added by zulipbot] labels May 8, 2021
@Ezio-Sarthak Ezio-Sarthak force-pushed the latex-rendering-fix branch 3 times, most recently from 7ce46de to 7473e2d Compare May 8, 2021 13:39
@neiljp
Copy link
Collaborator

neiljp commented May 10, 2021

@Ezio-Sarthak Given discussion in the stream, it seems like we have the source available in the rendered text and that may be the straightforward output to show for now - perhaps with a code styling? (we don't style the math at all right now, it seems?)

You've not updated the tests for math/katex, which is concerning that things are still passing; that aside I'd like to see some more detailed examples to cover the cases you discuss.

@neiljp neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label May 10, 2021
@Ezio-Sarthak Ezio-Sarthak force-pushed the latex-rendering-fix branch from 7473e2d to bc0d87f Compare May 13, 2021 15:27
@zulipbot zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels May 13, 2021
@Ezio-Sarthak
Copy link
Member Author

@zulipbot remove "PR awaiting update"
@zulipbot add "PR needs review"

@zulipbot zulipbot added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels May 16, 2021
neiljp
neiljp reviewed Jun 3, 2021
View reviewed changes
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Ezio-Sarthak This should definitely improve rendering, though without good math rendering directly (if possible!) I'm not sure how strong ZT is for maths. We should probably add an issue to indicate this limitation.

As per my inline comment I'd suggest replacing the existing test case with the new one since it more accurately reflects the behavior we want, and let's combine the first and second commits. I know I mentioned TDD, and it's good to note in the PR text to note you developed that way, but for now we're sticking with each commit passing CI without xfails - I think we discussed that, but we can follow-up on czo to discuss further if necessary.

This also needs a black rebase.

zulipterminal/config/themes.py Outdated Show resolved Hide resolved
tests/ui/test_ui_tools.py Outdated Show resolved Hide resolved
@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jun 3, 2021
@neiljp neiljp added this to the Next Release milestone Jun 3, 2021
@Ezio-Sarthak Ezio-Sarthak force-pushed the latex-rendering-fix branch 2 times, most recently from dc03444 to 744333b Compare July 5, 2021 08:19
@Ezio-Sarthak Ezio-Sarthak changed the title boxes: Avoid rendering multiple katex markups. Imrpove katex/math text rendering in ZT Jul 5, 2021
@Ezio-Sarthak Ezio-Sarthak added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 5, 2021
@Ezio-Sarthak Ezio-Sarthak changed the title Imrpove katex/math text rendering in ZT Improve katex/math text rendering in ZT Jul 5, 2021
@Ezio-Sarthak @neiljp
bugfix: boxes: Show raw math source instead of multiple katex output.
68b6bb0 
This commit stabilizes the behavior of katex/math text rendering in ZT.
Currently, a katex/math text is rendered multiple (mostly 3) times and
thus creates a visual problem. To sustain the markdown information, this
commit heads toward showing the raw KaTex/math source markup.

The existing tests are improved to reflect expected behavior. The cases
are distinguished as per the markdown (see inline comments).
@Ezio-Sarthak @neiljp
boxes/themes: Add styles to math (KaTeX) text for different themes.
99359a3 
Tests amended.
@neiljp neiljp force-pushed the latex-rendering-fix branch from 744333b to 99359a3 Compare July 6, 2021 23:01
@neiljp neiljp added PR ready to be merged PR has been reviewed & is ready to be merged and removed PR needs review PR requires feedback to proceed labels Jul 6, 2021
@neiljp
Copy link
Collaborator

neiljp commented Jul 6, 2021

@Ezio-Sarthak I just made a few text adjustments and re-pushed here, and will merge shortly 🎉

Other than the reduced noise from the math in triplicate, the styling separates the text well too 👍

@neiljp neiljp merged commit bee39b8 into zulip:main Jul 6, 2021
@Ezio-Sarthak Ezio-Sarthak mentioned this pull request Jul 7, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp neiljp left review comments

Assignees
No one assigned
Labels
area: message rendering PR ready to be merged PR has been reviewed & is ready to be merged size: M [Automatic label added by zulipbot]
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Ezio-Sarthak @neiljp @zulipbot