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

Enable Comrak support for style alerts in Markdown #10429

Closed
wants to merge 3 commits into from

Conversation

kbdharun
Copy link

@kbdharun kbdharun commented Jan 21, 2025

This PR enables alerts (for GitHub style alerts kivikakk/comrak#519) and multiline_block_quotes (for GitLab multiline blockquotes kivikakk/comrak#521) Comrak extensions that were added/modified in the recent release https://github.com/kivikakk/comrak/releases/tag/v0.34.0, to the Crates Markdown renderer.

Related discussion: #8506


While I was able to build the frontend as well as backend via Docker, I have not tested the rendering with a sample crates package (due to my unfamiliarity with the codebase). Any help with testing this and adding test cases for style alerts would be appreciated.

Extension docs:

https://docs.rs/comrak/0.35.0/comrak/struct.ExtensionOptionsBuilder.html#method.alerts
https://docs.rs/comrak/0.35.0/comrak/struct.ExtensionOptionsBuilder.html#method.multiline_block_quotes

@kbdharun kbdharun changed the title Enable Comark support for style alerts in Markdown Enable Comrak support for style alerts in Markdown Jan 21, 2025
Signed-off-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
@Turbo87
Copy link
Member

Turbo87 commented Jan 21, 2025

Any help with testing this and adding test cases for style alerts would be appreciated.

we need two test cases:

  1. checking the conversion of the markdown alert into an HTML alert. this should be added at the bottom of the file in the crates_io_markdown crate, which already has a bunch of similar tests.

  2. checking the styling of the HTML alerts. you can find a basic smoke test for the rendering at https://github.com/rust-lang/crates.io/blob/main/tests/acceptance/readme-rendering-test.js. if you run pnpm start you should be able to visit http://localhost:4040/tests?filter=Acceptance%20|%20README%20rendering and see it in action. if you replace await percySnapshot(assert); with await this.pauseTest(); you can pause the test and manually check if the styling matches your expectation.

@Turbo87 Turbo87 added the C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works label Jan 21, 2025
@Turbo87
Copy link
Member

Turbo87 commented Feb 4, 2025

@kbdharun are you still working on this?

@kbdharun
Copy link
Author

kbdharun commented Feb 4, 2025

@kbdharun are you still working on this?

Yes, thanks for the reminder, will sync the PR with the base branch and test out the changes as suggested.

Signed-off-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
@kbdharun kbdharun marked this pull request as draft February 4, 2025 18:51
@kbdharun
Copy link
Author

kbdharun commented Feb 4, 2025

Hi @Turbo87, I tried adding a test case but unfortunately it isn't functioning properly with the alerts extension, if possible can you assist fixing it. Thanks in advance.

References: https://github.com/kivikakk/comrak/blob/main/src/tests/fixtures/alerts.md, https://github.com/kivikakk/comrak/blob/main/src/tests/fixtures/multiline_blockquote.md

@Turbo87
Copy link
Member

Turbo87 commented Feb 5, 2025

I tried adding a test case but unfortunately it isn't functioning properly with the alerts extension, if possible can you assist fixing it.

for the Rust test: I don't think the test is wrong, it just shows that the alert extension isn't working as intended. Most likely the issue is related to the html_sanitizer in the same file, which only allows a certain set of tags, attributes and values. It would probably have to be adjusted to allow the corresponding CSS classes on these elements.

for the JS test: you will probably have to adjust the assertions to make that test pass.

@Turbo87
Copy link
Member

Turbo87 commented Feb 25, 2025

feel free to reopen if you feel like picking this back up :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants