Allow asides titles to contain markdown formatting#2126
Allow asides titles to contain markdown formatting#2126delucis merged 8 commits intowithastro:mainfrom
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. |
delucis
left a comment
There was a problem hiding this comment.
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 <swithinbank@gmail.com>
delucis
left a comment
There was a problem hiding this comment.
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.
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com> Co-authored-by: Essential Randomness <essential.randomn3ss@gmail.com>
Description
Fixes a bug where
asidetitles 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-stringas a dependency. You already hadhast-util-to-stringso 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.