Skip to content
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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Add <Badge> component #1530

wants to merge 27 commits into from

Conversation

kevinzunigacuellar
Copy link
Sponsor Member

@kevinzunigacuellar kevinzunigacuellar commented Feb 18, 2024

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" />

Copy link

changeset-bot bot commented Feb 18, 2024

🦋 Changeset detected

Latest commit: 4cbf409

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

Copy link

vercel bot commented Feb 18, 2024

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

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview May 8, 2024 2:28pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
starlight-i18n ⬜️ Ignored (Inspect) May 8, 2024 2:28pm

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Feb 18, 2024
@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Feb 18, 2024

size-limit report 📦

Path Size
/index.html 6.59 KB (+7.96% 🔺)
/_astro/*.js 22.02 KB (0%)
/_astro/*.css 13.24 KB (-1.62% 🔽)

@HiDeoo
Copy link
Member

HiDeoo commented Feb 22, 2024

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:

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

@kevinzunigacuellar
Copy link
Sponsor Member Author

kevinzunigacuellar commented Feb 22, 2024

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>

Copy link
Member

@HiDeoo HiDeoo left a 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 the class 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) I talked a bit with Chris who suggested that we could actually support all valid HTML attributes for <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 the Icon section
  • Remove the docs/src/content/docs/reference/test.mdx file

Does this sound correct to you? Do you see anything missing?

packages/starlight/user-components/Badge.astro Outdated Show resolved Hide resolved
packages/starlight/user-components/Badge.astro Outdated Show resolved Hide resolved
packages/starlight/user-components/Badge.astro Outdated Show resolved Hide resolved
packages/starlight/user-components/Badge.astro Outdated Show resolved Hide resolved
packages/starlight/user-components/Badge.astro Outdated Show resolved Hide resolved
docs/src/content/docs/guides/components.mdx Outdated Show resolved Hide resolved
docs/src/content/docs/guides/components.mdx Outdated Show resolved Hide resolved
docs/src/content/docs/guides/components.mdx Outdated Show resolved Hide resolved
docs/src/content/docs/guides/components.mdx Outdated Show resolved Hide resolved
--sl-color-bg-badge: transparent;
--sl-color-border-badge: currentColor;
color: inherit;
}
Copy link
Member

@martrapp martrapp Apr 18, 2024

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.

@HiDeoo
Copy link
Member

HiDeoo commented Apr 18, 2024

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.

@kevinzunigacuellar
Copy link
Sponsor Member Author

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 💜

@HiDeoo
Copy link
Member

HiDeoo commented Apr 20, 2024

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

@kevinzunigacuellar
Copy link
Sponsor Member Author

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

@HiDeoo
Copy link
Member

HiDeoo commented Apr 22, 2024

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 👍

@HiDeoo
Copy link
Member

HiDeoo commented Apr 23, 2024

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

@kevinzunigacuellar
Copy link
Sponsor Member Author

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

@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Apr 29, 2024

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.

Copy link
Member

@HiDeoo HiDeoo left a 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.

packages/starlight/schemas/badge.ts Show resolved Hide resolved
import { type Badge, BadgeComponentSchema } from '../schemas/badge';
import { parseWithFriendlyErrors } from '../utils/error-map';

type Props = Badge;
Copy link
Member

@HiDeoo HiDeoo Apr 29, 2024

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.

@HiDeoo
Copy link
Member

HiDeoo commented May 3, 2024

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?

@kevinzunigacuellar
Copy link
Sponsor Member Author

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

@HiDeoo
Copy link
Member

HiDeoo commented May 3, 2024

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
Copy link
Member

HiDeoo commented May 7, 2024

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:

  • 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?

@kevinzunigacuellar
Copy link
Sponsor Member Author

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

Copy link
Member

@HiDeoo HiDeoo left a 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 👏

packages/starlight/schemas/badge.ts Outdated Show resolved Hide resolved
packages/starlight/user-components/Badge.astro Outdated Show resolved Hide resolved
packages/starlight/user-components/Badge.astro Outdated Show resolved Hide resolved
Co-authored-by: HiDeoo <494699+HiDeoo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package 📚 docs Documentation website changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants