Skip to content

Add support for rendering alerts in README #11441

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
Merged
merged 2 commits into from
Jul 1, 2025
Merged

Add support for rendering alerts in README #11441

merged 2 commits into from
Jul 1, 2025

Conversation

eth3lbert
Copy link
Contributor

@eth3lbert eth3lbert commented Jun 26, 2025 •
edited
Loading

This PR continues the work on #10429 and makes it functional by adding allowed_classes. This ensures that the generated alert blocks won't be cleaned by the sanitizer. Tests have also been added to confirm everything works as expected.

Thanks, @kbdharun, for the work on #10429.

preview:
image
image

@eth3lbert eth3lbert force-pushed the markdown-render-alert branch from 5ab7ab3 to 364dd2b Compare June 26, 2025 15:28
@eth3lbert eth3lbert changed the title Add support for rendering alerts in markdown Add support for rendering alerts in README Jun 26, 2025
@eth3lbert eth3lbert force-pushed the markdown-render-alert branch 2 times, most recently from 49e99ee to 0bec9ee Compare June 26, 2025 18:00
@eth3lbert eth3lbert added the C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works label Jun 26, 2025
@eth3lbert
Copy link
Contributor Author

eth3lbert commented Jun 26, 2025 •
edited
Loading

Currently, alert icon support is handled via pure CSS for the following reasons:

  • Comrak does not currently insert alert icon nodes.
  • While we could somehow insert svg alert icon with a custom renderer, our sanitizer will remove SVG. :/ We could still use this method to inject a placeholder node and load icon via CSS, though. (the point would be: how important is it to have a placeholder node for the alert icon in the rendered HTML?)

There's also an alternative solution which doesn't require serving the SVG icon via CSS. This might require parsing the sanitized document and inserting the SVG into it.

eth3lbert
eth3lbert commented Jun 26, 2025
View reviewed changes
crates/crates_io_markdown/lib.rs
@@ -429,7 +443,7 @@ mod tests {
#[test]
fn text_with_forbidden_class_attribute() {
let text = "<p class='bad-class'>Hello World!</p>";
assert_snapshot!(markdown_to_html(text, None, ""), @"<p>Hello World!</p>");
assert_snapshot!(markdown_to_html(text, None, ""), @r#"<p class="">Hello World!</p>"#);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be caused by the allowed_classes. I'm not sure if there's any way to get around it.

@eth3lbert eth3lbert marked this pull request as ready for review June 26, 2025 18:20
@Turbo87
Copy link
Member

Turbo87 commented Jun 30, 2025

LGTM, though in the screenshot it looks like there is a lot of padding at the bottom of the alert. is that intentional? I couldn't find anything in the CSS that would explicitly set it like this. 🤔

@eth3lbert
Copy link
Contributor Author

LGTM, though in the screenshot it looks like there is a lot of padding at the bottom of the alert. is that intentional? I couldn't find anything in the CSS that would explicitly set it like this. 🤔

Ah, good catch! It shouldn't have a margin-bottom for the last child.

@eth3lbert eth3lbert force-pushed the markdown-render-alert branch from 0bec9ee to 2c8ff86 Compare June 30, 2025 12:56
@eth3lbert eth3lbert force-pushed the markdown-render-alert branch from 2c8ff86 to 26d3e61 Compare June 30, 2025 12:56
@eth3lbert
Copy link
Contributor Author

Thanks for the review! All the feedback should be addressed now. (I also rebased to the latest) :D

Turbo87
Turbo87 approved these changes Jun 30, 2025
View reviewed changes
Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

that looks better, thanks! :)

@Turbo87 Turbo87 merged commit 721b46e into rust-lang:main Jul 1, 2025
11 checks passed
@eth3lbert eth3lbert deleted the markdown-render-alert branch July 1, 2025 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Turbo87 Turbo87 Turbo87 approved these changes

Assignees
No one assigned
Labels
C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eth3lbert @Turbo87