-
-
Notifications
You must be signed in to change notification settings - Fork 814
Aside: Support custom icons #2261
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
Conversation
🦋 Changeset detectedLatest commit: 84f8780 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hello! Thank you for opening your first PR to Starlight! ✨ Here’s what will happen next:
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
6f13b2b
to
ba8211d
Compare
Astro helps you build faster websites with [“Islands Architecture”](https://docs.astro.build/en/concepts/islands/). | ||
::: | ||
``` | ||
|
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.
I'm not sure whether this should have a different header of its own or should we just specify it along with the custom aside title markdown
ba8211d
to
999fbce
Compare
999fbce
to
da7a1b7
Compare
Bumping this up for review, thanks! |
da7a1b7
to
c1029ae
Compare
Bumping this up for a review, thanks :) ! |
Really sorry for the delayed response. As we have been thinking and exploring support for custom icons in Starlight (e.g. from Iconify icon sets and also local ones), we also have been investigating how such support for aside icons could be implemented to make sure there would be no conflict or breaking changes between your PR and later, when we implement custom icons. I'll make sure to bring this up with Chris next time we chat, so we can get back to you with a concrete plan and a review of your PR as soon as possible. |
thank you for the update! |
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 for the amazing contribution and sorry again for the delay in getting back to you 🙌
After discussing and exploring custom icons in Starlight (e.g. from Iconify icon sets and also local ones), it looks we should be able to safely move on with your PR and later on, build on top of it to also support more custom icons in asides too.
I'm sharing a first review, mostly focused on the technical side of things, rather than the documentation, but this should help us get started in finalizing your great work.
Let me know if you have any questions or need any help with some points, do not hesitate to ask.
c1029ae
to
8a981cc
Compare
Only built-icons will be supported for now. This commit does not add support for the `:::` markdown shorthand for defining an aside, that will be done in the next commit.
8a981cc
to
1b6ade5
Compare
@HiDeoo Thank you for the detailed review:) I've addressed the review comments and made appropriate changes |
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.
Amazing update 🙌 Thanks a lot for all your work 🌟 (love the new test you added for icons with multiple paths 👌)
Left a few tiny comments/suggestions but most of them are quite tiny wording changes or missing imports, nothing major.
* main: (26 commits) [ci] release (withastro#3296) Fix slug override with `/` value (withastro#3293) i18n(fr): update `guides/i18n.mdx` (withastro#3294) i18n(ko-KR): update `i18n.mdx` (withastro#3292) [ci] release (withastro#3286) Revert withastro#3281 (withastro#3291) i18n(de): update `guides/i18n.mdx` (withastro#3289) Fix Astro i18n config default locale issue (withastro#3288) docs: fix `t.exists()` documentation + example (withastro#3287) Make targeting sidebar links with CSS a little easier (withastro#3281) i18n(fr): update `resources/plugins.mdx` (withastro#3284) Extract main padding to CSS custom property (withastro#3282) i18n(de): update plugins translation (withastro#3285) i18n(ko-KR): update `plugins.mdx` (withastro#3283) Add link to the codeblock-fullscreen plugin (withastro#3279) Fix TabItem typo in zh-cn authoring-content.mdx (withastro#3268) (withastro#3269) [ci] format i18n(ru): update translations (withastro#3270) Update `sharp` in docs & examples to latest (withastro#3261) Add missing danish UI translations (withastro#3252) ...
Hope you won't mind but I just updated the branch and also pushed the last suggestions I made during my last review so we can more easily evaluate with the team what remains to be done exactly before we can release your amazing work. |
* main: Small updates to `tailwind` template (withastro#3303) docs: showcase `astro-d2` and "Starlight Plugins by Example" (withastro#3302) [ci] release (withastro#3301) Fix `absolutePathToLang()` issue (withastro#3298)
* main: Exclude banner content from Pagefind indexing (withastro#3276) Restrict remark/rehype plugins usage (withastro#3274) [ci] release (withastro#3307) Fix Astro i18n default locale regression (withastro#3306) i18n(de): translate `plugins.mdx` and `community-content.mdx` (withastro#3304)
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 for your amazing work and your patience 🙌
Hope you won't mind us tweaking some last details so we can push this over the finish line, but it now looks perfect to me and ready to released in our next minor version 🎉 🚀
Co-authored-by: HiDeoo <[email protected]> Co-authored-by: Chris Swithinbank <[email protected]>
Description
This PR adds the ability to pass icon attribute to the Aside component. This icon attribute can be any icon supported by starlight by default.