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
Add <Badge>
component
#1530
base: main
Are you sure you want to change the base?
Add <Badge>
component
#1530
Conversation
🦋 Changeset detectedLatest commit: 4cbf409 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
size-limit report 📦
|
Kevin and badges, as a huge badge consumer, I'm always happy to see your PRs and this one is definitely gonna remove a lot of duplicated code for me 🙌 I did not yet closely look at the code, but some general (and personal) thoughts:
If I understand correctly, the |
Thank you for the review @HiDeoo 💜
You are right. They do look a bit bigger, I will follow the same style as the inline code block which uses a smaller font size.
I have mixed feelings about exposing I am not sure what the best way to go about this. I could use some guidance here.
It would be nice to have more theme versions. A class would be a very nice addition to include more customizations. In terms of customizing current variants. I think the nicest DX would be to override a global variable similar to what VitePress does. Then After adding .custom-badge {
--sl-color-bg-badge: cyan;
--sl-color-border-badge: purple;
--sl-color-text-badge: red;
} And use it like <Badge class="custom-badge">custom</Badge> |
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.
Sorry it took me so long to get back to this PR after my first review and also for the new updates you have been working on since then. Super appreciate your patience and hard work on this. 🙌
I left a few new comments and suggestions on the code, nothing major. I'd also like to recap everything else remaining to be done so we can push this PR to the finish line.
Confirm that theI talked a bit with Chris who suggested that we could actually support all valid HTML attributes forclass
prop is a good idea (I'm personally in favor as I suggested it and have use-cases for it, but I'd like to hear more opinions)<span>
which I think is a great idea.Check if the new light mode styles are something we want to include (may need to check Starlight design documents)Regarding the new light mode styles, after talking with Chris, a follow-up PR would be better to discuss and implement them.- After discussing the new component and checking various design systems, one important aspect that Chris brought up was the font size of the badge. It looks like the most common approach when it comes to badges and font sizes is to usually provide a limited set of sizes instead of matching the current font-size. Some examples includes for example IBM’s Carbon, GitHub Primer, Adobe Spectrum, etc. For a first iteration, we could start with a similar approach and provide a limited set of sizes, e.g.
size?: 'small' | 'medium' | 'large'
(considering users would still be able to override this with custom CSS if needed). - Update the branch so that we get the new updated
docs/src/content/docs/guides/components.mdx
file - Move the new section in
docs/src/content/docs/guides/components.mdx
just above theIcon
section - Remove the
docs/src/content/docs/reference/test.mdx
file
Does this sound correct to you? Do you see anything missing?
--sl-color-bg-badge: transparent; | ||
--sl-color-border-badge: currentColor; | ||
color: inherit; | ||
} |
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.
After having looked into this PR as the perfect solution for #1764 and martrapp/astro-vtbot#77, I would like to suggest to move the sidebar specific styling to
. There it would be a neighbor of the other aria-current='page'-styling, and you would get rid of the references to sidebar-content here.
Thanks for the amazing update to the PR. I did not check yet all the new code since my last review but sharing here the feedback we got from everyone who joined the Talking & Doc'ing session today.
Without too much thinking, I guess this could be a special case we could handle and adjust the badge position a little bit higher. Do you have any thoughts on this?
My initial thought was that if user really want to do this, they could fallback to a syntax like: test <sup><Badge text="Official" /></sup> After reading the I think I'm leaning towards not supporting this use case for now (considering this should still be doable using a custom CSS class) but would be happy to hear any feedback on this. |
I think it would be a bit complicated to handle the vertical alignment for user because it depends in two things:
I think baseline is a good default, users can always pass in a custom class to tweak their designs if they need to. Thanks again for the review @HiDeoo 💜 |
Are you referring to my first point, the second point, or both? |
Second point, I don't think we should support position. |
Ah perfect, thanks for the clarification. Then yeah, I think we totally agree and should only try to improve the first one 👍 |
Had a bit of time to play with the heading issue people mentioned during Talking & Doc'ing. It looks like it's mostly something very visible with larger headings. I wonder if just adding a rule close to something like this could help a bit: /* Badge in headings */
:global(.sl-markdown-content :is(h1, h2, h3, h4, h5, h6)) .sl-badge {
vertical-align: middle;
} Here is a preview before and after the change. Do you have any thoughts on this? I think this may be a bit better and less something that catches the eye. |
It looks good 💜, middle in only heading might work because of the bigger font sizes. |
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
|
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. I just pushed this heading changes so we can have a preview including it.
I left a few comments regarding your latest changes, very minor, but this is looking really good. I would guess the documentation may still need some work but feature-wise and UI-wise I am personally happy with it.
import { type Badge, BadgeComponentSchema } from '../schemas/badge'; | ||
import { parseWithFriendlyErrors } from '../utils/error-map'; | ||
|
||
type Props = Badge; |
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.
At the moment, this is equivalent to z.output<ReturnType<typeof badgeSchema>>
but I guess we would want the props to be z.input<typeof BadgeComponentSchema>
instead so we may need to create a type for this new component schema.
Tried to work a bit on the documentation as I was personally not super happy with the "custom badges" section as we usually have examples which includes a demo but coming up with a concise example where the demo would be looking good in both light and dark mode is a bit tricky imo. To do that, I rewrote some parts and sharing it here at the moment as it's only a rough draft and want to get opinions first. ### Badges
import { Badge } from '@astrojs/starlight/components';
Use the `<Badge>` component to display small pieces of information, such as status or labels.
Pass the content you want to display to the `text` attribute of the `<Badge>` component.
Optionally, you can specify a `variant` attribute with values like `tip`, `note`, `caution`, `danger`, `success` or `default` (the default using the theme accent color) to adjust the badge's appearance.
To adjust the badge's size, you can specify a `size` attribute with values like `small` (the default), `medium`, or `large`.
You can also include other `<span>` attributes such as `class`.
```mdx title="src/content/docs/example.mdx"
import { Badge } from '@astrojs/starlight/components';
<Badge text="New" variant="tip" size="small" />
<Badge text="Deprecated" variant="caution" size="medium" />
<Badge text="Starlight" variant="note" size="large" />
```
The code above generates the following on the page:
<Badge text="New" variant="tip" size="small" />
<Badge text="Deprecated" variant="caution" size="medium" />
<Badge text="Starlight" variant="note" size="large" /> Altho, this made me realize some points:
What do you think? |
|
Amazing, love to hear that.
Or just
Yeah, that's what I tried to do in my rewritten version with the "You can also include other |
Thanks for the new update 🎉 I think it's looking good, and how you were able to reduce the documentation shows that imo. One thing I noticed:
Any thoughts? |
I see what you meant. I have applied the requested changes. Thanks for the through review @HiDeoo |
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. I left 2 small suggestions to simplify a little bit the code and avoid some duplication, but personally I'm very happy with the new component 🎉
You did an amazing work 👏
Co-authored-by: HiDeoo <494699+HiDeoo@users.noreply.github.com>
Description
<Badge>
component to @astrojs/starlight/components module.Notes
Move outline style to be applied with css only to the sidebar
current-aria="page"
Add global variables to edit badge variants
Add prop
"size"
(small
,medium
,large
) to edit badge sizes (font size and padding).Adds
class
prop for injecting custom variables