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

Fixes #1884 - Support Spoiler Tags #3018

Merged
merged 8 commits into from
Jun 14, 2023

Conversation

ninanator
Copy link
Contributor

@ninanator ninanator commented Jun 11, 2023

WHAT & WHY

Adds support for managing spoiler blocks by:

  • Switching to markdown-it.rs, a Rust port of themarkdown-it library used by the UI and, more importantly, supports adding and creating custom plugins.
    • Unfortunately, the library comrak nor another potentially performant library pulldown-cmark do not appear to have any support for plugins so the library switch is necessary.
  • Adding a custom spoiler plugin to handle parsing spoiler tags.
  • Writing basic tests to ensure that nothing should change due to the library swap. If the library needs to be changed again later, it should help with backwards compatibility checking.

This is my first time writing in Rust, so apologies for any obvious semantic mistakes 🙏

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Thanks for this!

AFAIK this will only affect RSS feeds and some federated objects. The lemmy UIs just receive pure markdown, so they have to have do their own markdown parsers. I still have yet to add some of this stuff into kotlin / jerboa.

crates/utils/src/utils/markdown.rs Show resolved Hide resolved
crates/utils/src/utils/markdown/spoiler_rule.rs Outdated Show resolved Hide resolved
@Nutomic
Copy link
Member

Nutomic commented Jun 12, 2023

FYI this will affect RSS feeds, emails, and federation (except with other Lemmy instances in which case the markdown source is used). Nothing else I think.

@ninanator ninanator requested review from dessalines and Nutomic June 13, 2023 02:29
@ninanator
Copy link
Contributor Author

@Nutomic @dessalines Thanks for the initial reviews; made some changes based on your feedback. LMK what you think!

@Nutomic Nutomic enabled auto-merge (squash) June 13, 2023 20:30
@Nutomic
Copy link
Member

Nutomic commented Jun 13, 2023

Thank you!

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Thanks!

@Nutomic Nutomic disabled auto-merge June 14, 2023 11:15
@Nutomic Nutomic merged commit 1c7bfd6 into LemmyNet:main Jun 14, 2023
@phiresky
Copy link
Collaborator

phiresky commented Jul 5, 2023

This change is causing issues since markdown-it panics in some cases which breaks feeds and federation. See also #3411 and markdown-it-rust/markdown-it#26

Edit: I also want to mention since this is not mentioned anywhere: This seems to "just" be a subset of fenced_divs from pandoc, which there might be another existing implementation using a different markdown parser for.

@Nutomic
Copy link
Member

Nutomic commented Jul 5, 2023

Looks like the maintainer confirmed that this is a bug in upstream, not related to this change.

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.

4 participants