starlight icon indicating copy to clipboard operation
starlight copied to clipboard

Add `<Badge>` component

Open kevinzunigacuellar opened this issue 1 year ago • 21 comments

Description

  • Adds an <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

    .custom-badge {
      --sl-color-bg-badge: red;
      --sl-color-border-badge: blue;
      --sl-color-text-badge: cyan;
    }
    
    <Badge class="custom-badge" text="custom" />
    

kevinzunigacuellar avatar Feb 18 '24 21:02 kevinzunigacuellar

🦋 Changeset detected

Latest commit: a9aee3dec448d5802c0f4c4c163333a888e0b6a3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Minor

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

changeset-bot[bot] avatar Feb 18 '24 21:02 changeset-bot[bot]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Jun 5, 2024 1:42pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
starlight-i18n ⬜️ Ignored (Inspect) Jun 5, 2024 1:42pm

vercel[bot] avatar Feb 18 '24 21:02 vercel[bot]

size-limit report 📦

Path Size
/index.html 5.79 KB (+10.01% 🔺)
/_astro/*.js 21.81 KB (0%)
/_astro/*.css 13.57 KB (-1.56% 🔽)

astrobot-houston avatar Feb 18 '24 21:02 astrobot-houston

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 :raised_hands:

I did not yet closely look at the code, but some general (and personal) thoughts:

  • Overall, I find the badges/font a bit too big outside of the sidebar. I usually find the Vitepress version too small but I think the current version is the opposite and wonder if we could find a middle ground. Note that I'm usually not the best person to ask for design feedback, so take this with a grain of salt.
  • Would it make sense to also provide the outline variant to users?
  • I wonder if allowing a class prop would be nice to allow users to customize the badge even further. To explain, some of the use cases I encountered where a custom class would help:
    • Defining more custom colors, for example in the requests/OpenAPI land, there are more request types than variants we have, so being able to define a custom color would be nice.
    • Having badges themed to a brand color for a company or project.

Is an update in the package.json required?

If I understand correctly, the packages/starlight/components.ts update should be enough.

HiDeoo avatar Feb 22 '24 08:02 HiDeoo

Thank you for the review @HiDeoo 💜

Overall, I find the badges/font a bit too big outside of the sidebar. I usually find the Vitepress version too small but I think the current version is the opposite and wonder if we could find a middle ground. Note that I'm usually not the best person to ask for design feedback, so take this with a grain of salt.

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.

Would it make sense to also provide the outline variant to users?

I have mixed feelings about exposing outline. it is not an official variant since it's only applied when the sidebar is active. But then I don't see why it couldn't be an official one.

I am not sure what the best way to go about this. I could use some guidance here.

I wonder if allowing a class prop would be nice to allow users to customize the badge even further. To explain, some of the use cases I encountered where a custom class would help: Defining more custom colors, for example in the requests/OpenAPI land, there are more request types than variants we have, so being able to define a custom color would be nice. Having badges themed to a brand color for a company or project.

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 class could override variables locally resulting in a new badge theme.

After adding class prop it should the user should be able to create a new theme like this

.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>

kevinzunigacuellar avatar Feb 22 '24 19:02 kevinzunigacuellar

Thanks for the review @HiDeoo. I added the changes you suggested, I don't think it's missing anything else besides from doing some more testing for CSS edge cases. I will do some local testing to see what pops up.

kevinzunigacuellar avatar Apr 16 '24 17:04 kevinzunigacuellar

We're going to review this today on Talking and Doc'ing @kevinzunigacuellar ! :raised_hands:

sarah11918 avatar Apr 18 '24 10:04 sarah11918

Additional note that this PR will also close #1764 by removing the outline CSS class which collides with Tailwind CSS outline utility adding outline-style: solid; and leading to a thicker outline for sidebar badges for the current page:

SCR-20240418-kfwo

HiDeoo avatar Apr 18 '24 14:04 HiDeoo

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.

  1. The first point mentioned by pretty much everyone is the case of badges in titles. Even tho this may not be an usage we want to encourage, we should still be able to provide a look and feel that most of people would expect. With the current implementation, the badge font baseline matches the title font baseline but this makes the badges feel too low compared to the title.
image

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?

  1. Another point mentioned was the possibility of using badges as superior lettering.

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 <sup> documentation tho, this is not a valid usage of <sup> but instead, for this specific case, something like vertical-align: super should be used. This would mean having a new prop to support this use case and adjust the badge position accordingly (and also potentially adjusting the font size in this case).

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.

HiDeoo avatar Apr 18 '24 15:04 HiDeoo

I think it would be a bit complicated to handle the vertical alignment for user because it depends in two things:

  1. the size of the badge
  2. the size of the parent font.

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 💜

kevinzunigacuellar avatar Apr 19 '24 15:04 kevinzunigacuellar

Are you referring to my first point, the second point, or both?

HiDeoo avatar Apr 20 '24 08:04 HiDeoo

Second point, I don't think we should support position.

kevinzunigacuellar avatar Apr 20 '24 22:04 kevinzunigacuellar

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 :thumbsup:

HiDeoo avatar Apr 22 '24 08:04 HiDeoo

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.

headings

HiDeoo avatar Apr 23 '24 16:04 HiDeoo

It looks good 💜, middle in only heading might work because of the bigger font sizes.

kevinzunigacuellar avatar Apr 26 '24 14:04 kevinzunigacuellar

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

Locale File Note
en guides/components.mdx Source changed, localizations will be marked as outdated.
en guides/sidebar.mdx Source changed, localizations will be marked as outdated.
en reference/configuration.mdx Source changed, localizations will be marked as outdated.
en reference/frontmatter.md Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

astrobot-houston avatar Apr 29 '24 09:04 astrobot-houston

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:

  • We forgot Chris suggestion to support all valid HTML attributes for <span>.
    • For consistency, we should probably remove the class attribute from the schema and use the same approach as sidebar link custom HTML attributes and use an attrs property. Or we could also decide supporting all attributes is not super useful on sidebar badges compared to the component and only keep the class for sidebar links while the component support all <span> attributes?
  • We can probably stop using set:html.

What do you think?

HiDeoo avatar May 03 '24 10:05 HiDeoo

  • I like the idea of keeping class for sidebar and attrs for components.
  • I completely forgot about that set:html, definitely changing that for a set:text.
  • I wasn't very happy with the custom badges section, too. By including attributes I think it would be easier to explain that the user may override this component however they see fit.

kevinzunigacuellar avatar May 03 '24 17:05 kevinzunigacuellar

Amazing, love to hear that.

I completely forgot about that set:html, definitely changing that for a set:text.

Or just <span class:list={['sl-badge', variant, size, className]}>{text}</span>. Considering this is equivalent and way more common, it could potentially be more readable for others.

I wasn't very happy with the custom badges section, too. By including attributes I think it would be easier to explain that the user may override this component however they see fit.

Yeah, that's what I tried to do in my rewritten version with the "You can also include other <span> attributes such as class." sentence. It's a pattern we have used for Link Cards. Of course, it could be improved as I'm not the best at documentation but I think we are on the right track.

HiDeoo avatar May 03 '24 17:05 HiDeoo

Thanks for the new update :tada:

I think it's looking good, and how you were able to reduce the documentation shows that imo. One thing I noticed:

  • I mentioned attrs as a parallel with sidebar link custom HTML attributes, so that would only apply to the sidebar (not the component) but as we decided to not support that, I think we could drop entirely the new attrs. This would mean that in the sidebar a user can use class (this is currently the case) and on the component and a user could directly use <Badge text="New" style="…" />.
  • As we would not support all attributes for sidebar badges (only class), I think it would be fine to just not include attributes in the Zod schema at all, pass w/e extra attributes to the <span> without validating them when the user is using the component directly and have the component Props typed with something close to z.input<typeof badgeSideBarSchema> & HTMLAttributes<'span'>.
    • We basically would only validate variant, text and class and that's it. This would allow for quite a few simplifications in the schemas I assume.

Any thoughts?

HiDeoo avatar May 07 '24 07:05 HiDeoo

I see what you meant. I have applied the requested changes. Thanks for the through review @HiDeoo

kevinzunigacuellar avatar May 07 '24 14:05 kevinzunigacuellar

HYPE!

trueberryless avatar Jun 05 '24 17:06 trueberryless