-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,10 @@ Other content is also supported in asides. | |
</Aside> | ||
|
||
<Aside type="danger">Do not give your password to anyone.</Aside> | ||
|
||
<Aside icon="heart"> | ||
Aside with a custom icon. The icon must be part of starlight's icon list. | ||
</Aside> | ||
```` | ||
|
||
<Fragment slot="markdoc"> | ||
|
@@ -73,6 +77,10 @@ Other content is also supported in asides. | |
{% aside type="danger" %} | ||
Do not give your password to anyone. | ||
{% /aside %} | ||
|
||
{% aside icon="heart" %} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For Markdoc to support this new attribute, the aside: {
render: component('@astrojs/starlight/components', 'Aside'),
attributes: {
+ icon: {
+ type: String,
+ required: false,
+ },
title: {
type: String,
required: false,
},
type: {
type: String,
required: false,
default: 'note',
matches: ['note', 'danger', 'caution', 'tip'],
},
},
}, |
||
Aside with a custom icon. The icon must be part of starlight's icon list. | ||
{% /aside %} | ||
```` | ||
|
||
</Fragment> | ||
|
@@ -95,6 +103,10 @@ Do not give your password to anyone. | |
|
||
<Aside type="danger">Do not give your password to anyone.</Aside> | ||
|
||
<Aside icon="heart"> | ||
Aside with a custom icon. The icon must be part of starlight's icon list. | ||
</Aside> | ||
|
||
</Fragment> | ||
|
||
</Preview> | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -151,6 +151,20 @@ Astro helps you build faster websites with [“Islands Architecture”](https:// | |||||
::: | ||||||
``` | ||||||
|
||||||
### Custom aside icons | ||||||
|
||||||
You can specify a custom icon for the aside in curly brackets following the aside type/title, e.g. `:::tip[Did you know?]{icon="heart"}`. | ||||||
|
||||||
:::tip[Did you know?]{icon="heart"} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possible consideration to think about: maybe we should only focus on icons here and have a basic example not using a custom title too so users don't think you need a custom title to have a custom icon?
Suggested change
Any thoughts? |
||||||
Astro helps you build faster websites with [“Islands Architecture”](https://docs.astro.build/en/concepts/islands/). | ||||||
::: | ||||||
|
||||||
```md | ||||||
:::tip[Did you know?]{icon="heart"} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May need to be updated depending on the decision regarding my previous comment. |
||||||
Astro helps you build faster websites with [“Islands Architecture”](https://docs.astro.build/en/concepts/islands/). | ||||||
::: | ||||||
``` | ||||||
|
||||||
### More aside types | ||||||
|
||||||
Caution and danger asides are helpful for drawing a user’s attention to details that may trip them up. | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -126,6 +126,29 @@ Some text | |||||||||||||
); | ||||||||||||||
}); | ||||||||||||||
|
||||||||||||||
describe('custom icons', () => { | ||||||||||||||
test.each(['note', 'tip', 'caution', 'danger'])('%s with custom label', async (type) => { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably add an extra test with both custom labels + icons. |
||||||||||||||
const res = await processor.render(` | ||||||||||||||
:::${type}{icon="heart"} | ||||||||||||||
Some text | ||||||||||||||
::: | ||||||||||||||
`); | ||||||||||||||
expect(res.code).includes( | ||||||||||||||
'M20.16 5A6.29 6.29 0 0 0 12 4.36a6.27 6.27 0 0 0-8.16 9.48l6.21 6.22a2.78 2.78 0 0 0 3.9 0l6.21-6.22a6.27 6.27 0 0 0 0-8.84m-1.41 7.46-6.21 6.21a.76.76 0 0 1-1.08 0l-6.21-6.24a4.29 4.29 0 0 1 0-6 4.27 4.27 0 0 1 6 0 1 1 0 0 0 1.42 0 4.27 4.27 0 0 1 6 0 4.29 4.29 0 0 1 .08 6Z' | ||||||||||||||
); | ||||||||||||||
Comment on lines
+136
to
+138
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having the path inlined in the test does not provide a lot of value, maybe we could just use snapshot files in this case:
Suggested change
|
||||||||||||||
|
||||||||||||||
expect(async () => | ||||||||||||||
processor.render(` | ||||||||||||||
:::${type}{icon="invalid-icon-name"} | ||||||||||||||
Some text | ||||||||||||||
::: | ||||||||||||||
`) | ||||||||||||||
).rejects.toThrowError( | ||||||||||||||
'Failed to parse Markdown file "undefined":\nIcon name should be part of Starlight\'s icon list.' | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use |
||||||||||||||
); | ||||||||||||||
}); | ||||||||||||||
Comment on lines
+140
to
+149
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this test is logging the actual error to the console with a Vitest warning:
I'm assuming this is because the |
||||||||||||||
}); | ||||||||||||||
|
||||||||||||||
test('ignores unknown directive variants', async () => { | ||||||||||||||
const res = await renderMarkdown(` | ||||||||||||||
:::unknown | ||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,6 +15,10 @@ import remarkDirective from 'remark-directive'; | |||||||||||||||||
import type { Plugin, Transformer } from 'unified'; | ||||||||||||||||||
import { visit } from 'unist-util-visit'; | ||||||||||||||||||
import type { HookParameters, StarlightConfig } from '../types'; | ||||||||||||||||||
import { Icons } from '../components/Icons'; | ||||||||||||||||||
import { fromHtml } from 'hast-util-from-html'; | ||||||||||||||||||
import type { Element } from 'hast'; | ||||||||||||||||||
import { AstroError } from 'astro/errors'; | ||||||||||||||||||
|
||||||||||||||||||
interface AsidesOptions { | ||||||||||||||||||
starlightConfig: Pick<StarlightConfig, 'defaultLocale' | 'locales'>; | ||||||||||||||||||
|
@@ -159,6 +163,7 @@ function remarkAsides(options: AsidesOptions): Plugin<[], Root> { | |||||||||||||||||
return; | ||||||||||||||||||
} | ||||||||||||||||||
const variant = node.name; | ||||||||||||||||||
const attributes = node.attributes; | ||||||||||||||||||
if (!isAsideVariant(variant)) return; | ||||||||||||||||||
|
||||||||||||||||||
// remark-directive converts a container’s “label” to a paragraph added as the head of its | ||||||||||||||||||
|
@@ -180,6 +185,19 @@ function remarkAsides(options: AsidesOptions): Plugin<[], Root> { | |||||||||||||||||
node.children.splice(0, 1); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
let iconPath = iconPaths[variant]; | ||||||||||||||||||
if (attributes) { | ||||||||||||||||||
if (attributes['icon']) { | ||||||||||||||||||
Comment on lines
+189
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be able to avoid the 2 single
Suggested change
|
||||||||||||||||||
const iconName = attributes['icon'] as keyof typeof Icons; | ||||||||||||||||||
if (!Object.keys(Icons).includes(iconName)) { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We should be able to avoid looping through all keys by just checking if an icon for the provided Later down below, we can re-use |
||||||||||||||||||
throw new AstroError("Icon name should be part of Starlight's icon list."); | ||||||||||||||||||
} | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've put a placeholder error for now, please feel free to suggest another error message that might follow starlight's guidelines. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something like: throw new AstroError(
'Invalid aside icon',
`An aside custom icon must be set to the name of one of Starlight\’s built-in icons, but received \`${iconName}\`.\n\n` +
'See https://starlight.astro.build/reference/icons/#all-icons for a list of available icons.'
); Which would render as: ![]() |
||||||||||||||||||
const { properties } = fromHtml(Icons[iconName], { fragment: true }) | ||||||||||||||||||
.children[0] as Element; | ||||||||||||||||||
iconPath = [s('path', properties)]; | ||||||||||||||||||
Comment on lines
+195
to
+197
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely tricky, and also such approach would not work if the SVG has multiple children, e.g. 2 ![]() The above approach would render as such: ![]() What I have done in #3024 for this case is use the following approach:
Suggested change
to ensure we render everything properly with /** Hacky function that generates the children of an mdast SVG tree. */
function makeSvgChildNodes(children: Result['children']): any[] {
const nodes: P[] = [];
for (const child of children) {
if (child.type !== 'element') continue;
nodes.push({
type: 'paragraph',
data: { hName: child.tagName, hProperties: child.properties },
children: makeSvgChildNodes(child.children),
});
}
return nodes;
} with the With such approach, an icon like ![]() Note that we should definitely add a specific test with for such scenario. |
||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels a bit ugly-ish to me, any pointers to improve this would be welcome! |
||||||||||||||||||
const aside = h( | ||||||||||||||||||
'aside', | ||||||||||||||||||
{ | ||||||||||||||||||
|
@@ -197,7 +215,7 @@ function remarkAsides(options: AsidesOptions): Plugin<[], Root> { | |||||||||||||||||
fill: 'currentColor', | ||||||||||||||||||
class: 'starlight-aside__icon', | ||||||||||||||||||
}, | ||||||||||||||||||
iconPaths[variant] | ||||||||||||||||||
iconPath | ||||||||||||||||||
), | ||||||||||||||||||
...titleNode, | ||||||||||||||||||
]), | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -10,7 +10,7 @@ interface Props { | |||||||||||
title?: string; | ||||||||||||
} | ||||||||||||
Comment on lines
10
to
11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We would need to define the new prop in the
Suggested change
with an extra |
||||||||||||
|
||||||||||||
let { type = 'note', title } = Astro.props; | ||||||||||||
let { type = 'note', title, icon } = Astro.props; | ||||||||||||
|
||||||||||||
if (!asideVariants.includes(type)) { | ||||||||||||
throw new AstroError( | ||||||||||||
|
@@ -27,7 +27,7 @@ if (!title) { | |||||||||||
|
||||||||||||
<aside aria-label={title} class={`starlight-aside starlight-aside--${type}`}> | ||||||||||||
<p class="starlight-aside__title" aria-hidden="true"> | ||||||||||||
<Icon name={icons[type]} class="starlight-aside__icon" />{title} | ||||||||||||
<Icon name={icon || icons[type]} class="starlight-aside__icon" />{title} | ||||||||||||
</p> | ||||||||||||
<div class="starlight-aside__content"> | ||||||||||||
<slot /> | ||||||||||||
|
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.
We should probably move this new code to an entire new section, e.g.
Use custom icons
like we do for other components and mention the newicon
prop at the end of the page in the Props section.I've created a draft of such section in #3024 in this file if you want to take a look.