-
-
Notifications
You must be signed in to change notification settings - Fork 571
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
Allow asides titles to contain markdown formatting #2126
Conversation
🦋 Changeset detectedLatest commit: 7fc5570 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment was marked as outdated.
This comment was marked as outdated.
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hello! Thank you for opening your first PR to Starlight! ✨ Here’s what will happen next:
|
Thanks a lot for your contribution 🙌 I did not yet dive in detail into the changes, altho they seems pretty straightforward at first glance (except maybe the lock file changes, looks like quite a few deps got updated). One thing I'd like to point out and discuss before moving forward is the I guess there are several possibilities to handle this, e.g. only support this for the directive syntax like in the current implementation, or also support it for the component syntax. I think I'm personally leaning towards the latter, but I'd like to hear people's opinions on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @essential-randomness! This looks great.
Regarding @HiDeoo’s question about what to do about <Aside>
, that’s a great question!
My initial thought was “we can just require HTML there and use set:html
” — i.e. usage like
<Aside title="Custom <code>code</code> label">
But then we’ll still need to strip out the HTML for the aria-label
. That may still be preferable to supporting full-on Markdown rendering there though?
Also curious whether people think it’s OK to merge this before fixing support in <Aside>
?
I think so, this would align with the banner behavior for example and would be way more practical than let's say a slot.
I personally think it's fine to merge this first, my comment was more about pointing out the potential difference and discuss it rather than having it block this PR. |
Co-authored-by: Chris Swithinbank <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the PR @essential-randomness! Happy with this and then we can think about how to tackle <Aside>
support in a similar vein if and when it is needed.
Description
Fixes a bug where
aside
titles would get cut out if any markdown formatting was added in them.Markdown of aside with formatting:
Before the fix
After the fix
I took the liberty to add
mdast-util-to-string
as a dependency. You already hadhast-util-to-string
so I assumed this would be fine and better than writing bespoke code to handle nested formatting. Let me know if you want me to remove it.