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

feat(utils): markdown callouts #2298

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

Conversation

Erb3
Copy link
Contributor

@Erb3 Erb3 commented Aug 25, 2024

I've added markdown callouts, with the same syntax as GitHub alerts. Behind the scenes it uses markdown-it-github-alerts. It currently uses octicons as that is default in markdown-it-github-alerts, however they can be replaced if we have replacements.

Syntax

> [!NOTE]
> Highlights information that users should take into account, even when skimming.

> [!TIP]
> Optional information to help a user be more successful.

> [!IMPORTANT]
> Crucial information necessary for users to succeed.

> [!WARNING]
> Critical content demanding immediate user attention due to potential risks.

> [!CAUTION]
> Negative potential consequences of an action.

Screenshot

screenshot of how the callouts look

Tasks

  • Add markdown-it-github-alerts
  • Add basic styles for the callouts
  • Get it working™
  • Make XSS filter safe-ish again

@Erb3 Erb3 marked this pull request as ready for review August 25, 2024 17:40
@Erb3
Copy link
Contributor Author

Erb3 commented Aug 25, 2024

CI failure unrelated to this PR, a CI fix is available in #2296.

Copy link
Member

@Prospector Prospector left a comment

Choose a reason for hiding this comment

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

Looks good, though we should use Lucide icons for consistency and the bar on the left should be rounded to fit better

@Erb3
Copy link
Contributor Author

Erb3 commented Aug 27, 2024

Looks good, though we should use Lucide icons for consistency and the bar on the left should be rounded to fit better

I've moved to lucide icons, however the rounded bar is a bit harder. We are currently using border-radius on the div, and a border-left. I increased the border radius since I'm not sure where to go from here, but this only makes 2 of the bar corners rounded.

image

I don't think it looks terrible, but if we want to make the whole bar rounded we will have to take a difference approach. What are your thoughts?

@Prospector
Copy link
Member

to make the whole bar rounded, can just use a pseudoelement

@Minenash
Copy link
Contributor

I love this so much, thank you!

@Erb3
Copy link
Contributor Author

Erb3 commented Aug 28, 2024

to make the whole bar rounded, can just use a pseudoelement

Done

image

@Prospector
Copy link
Member

nice! thanks for contributing this, been meaning to add it for a very long time

packages/assets/styles/classes.scss Outdated Show resolved Hide resolved
@@ -24,6 +25,19 @@ export const configuredXss = new FilterXSS({
source: ['media', 'sizes', 'src', 'srcset', 'type'],
p: [...(whiteList.p || []), 'align'],
div: [...(whiteList.p || []), 'align'],
svg: [
Copy link
Member

Choose a reason for hiding this comment

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

this has me a little concerned as I know there's a bunch of random things you can do with svgs. it doesn't necessarily look like this allows anything bad but I don't know if I am knowledgable enough about svg vulnerabilities to approve this confidently.

it also seems to me like the elements added as part of the markdown renderer should not be subject to the xss/whitelisted elements as the raw html input. I wonder if there is a different way to do this?

Copy link
Contributor Author

@Erb3 Erb3 Aug 28, 2024

Choose a reason for hiding this comment

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

This was also a concern for me, but I frankly found no other way to do it. From my testing the jsxss was executed based on the output from markdown-it, not the description itself. We can probably change it to be pre-html, but that sounds insecure.

@@ -75,6 +89,28 @@ export const configuredXss = new FilterXSS({
}
return `${name}="${escapeAttrValue(allowedClasses.join(' '))}"`
}

// For markdown callouts
if (name === 'class' && ['div', 'p'].includes(tag)) {
Copy link
Member

Choose a reason for hiding this comment

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

again it seems weird that this is being whitelisted within the input, as someone could put these classes on their own html elements and have weird styles happen

@@ -129,6 +165,19 @@ export const md = (options = {}) => {
...options,
})

md.use(MarkdownItGitHubAlerts, {
icons: {
note: '<svg xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-info"><circle cx="12" cy="12" r="10"/><path d="M12 16v-4"/><path d="M12 8h.01"/></svg>',
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be possible to import the relevant icons from @modrinth/assets instead of hardcoding them here? A couple of them might need to be added to the library though

Copy link
Contributor Author

@Erb3 Erb3 Aug 28, 2024

Choose a reason for hiding this comment

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

I've added them to the modrinth/assets locally, however how would I go about using these? Markdown-it-github-alerts requires a string, can't take a component afaik. I guess I could import and export it as a string in assets, but it feels a bit jank

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants