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 support for translating sidebar badges #2285

Merged
merged 9 commits into from
Sep 18, 2024

Conversation

HiDeoo
Copy link
Member

@HiDeoo HiDeoo commented Sep 5, 2024

Description

This PR adds the ability to translate sidebar badges using the same approach as the site title. A badge text can be a string, or for multilingual sites, an object with values for each different locale.

starlight({
  sidebar: [
    {
      slug: "test",
      badge: {
        en: "New",
        "pt-BR": "Novo",
      },
    },
  ],
});

This syntax is compatible with the string/object syntax to define badges. To support this feature, a new type of badge was added (I18nBadge) and can only be used in the sidebar configuration.

Copy link

changeset-bot bot commented Sep 5, 2024

🦋 Changeset detected

Latest commit: dc863a8

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

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Sep 5, 2024
Copy link

netlify bot commented Sep 5, 2024

Deploy Preview for astro-starlight ready!

Name Link
🔨 Latest commit dc863a8
🔍 Latest deploy log https://app.netlify.com/sites/astro-starlight/deploys/66ea88c6b658ff0007c69e57
😎 Deploy Preview https://deploy-preview-2285--astro-starlight.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Sep 5, 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/sidebar.mdx 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.

Comment on lines 536 to 556
items: [
{
slug: 'constellations/andromeda',
badge: {
en: 'New',
'pt-BR': 'Novo',
},
},
{
slug: 'constellations/scorpius',
badge: {
text: {
en: 'Outdated',
'pt-BR': 'Descontinuado',
},
variant: 'caution',
},
},
],
},
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff! Happy to add this.

I wonder if it would be simpler to only support localized strings using the text property though? I find it a bit weird to have two config options that are objects but that mean very different things depending on what the keys are, i.e. one object is “record of locales to labels” the other is “precisely typed config object with known keys”. (Without testing, I’d guess this also interferes with IDE autocomplete?)

Also, imagine this badge:

badge: {
  en: 'New',
  fr: 'Nouveau',
}

If you decide you want to make it a success variant, this doesn’t work:

badge: {
  en: 'New',
  fr: 'Nouveau',
+  variant: 'success',
}

So overall, I think I’m in favour of a single syntax rather than two. What do you think?

Copy link
Member Author

@HiDeoo HiDeoo Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial implementation was what you described, then I somehow managed to convince myself that if you were using the shorthand notation, you would prefer to use that too with translated labels. I ended up not liking it personally but couldn't decide if I should remove it or not.

If you're having the same feeling too, I'll be more than happy to remove it (and it'll make the implementation simpler). I'll update the PR accordingly. Thanks again for the great input 🙌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, yeah, going from badge: 'New' to badge: { text: { en: 'New' } } feels superficially like a lot, but I think in practice it’s OK and even it is slightly verbose, it definitely is clearer to explain/understand. Plus once you have a couple of locales the difference is kind of not so bad, e.g.

badge: { text: { en: 'New', fr: 'Nouveau', de: 'Neu', it: 'Nuovo' } }
// vs
badge: { en: 'New', fr: 'Nouveau', de: 'Neu', it: 'Nuovo' }

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second having only one config option with locales inside text. It's a key longer but a very small price to pay to have localized badges 💜.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you both, super appreciated. I have updated the PR accordingly, definitely an improvement 👍

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @HiDeoo — ready for the next minor 🫡

Comment on lines +488 to +494
if (!defaultText) {
throw new AstroError(
`The badge text for "${itemLabel}" must have a key for the default language "${defaultLang}".`,
'Update the Starlight config to include a badge text for the default language.\n' +
'Learn more about sidebar badges internationalization at https://starlight.astro.build/guides/sidebar/#internationalization-with-badges'
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that one way to avoid needing this would be to use the same API we use for sidebar items themselves:

{
  label: 'Andromeda',
  translations: { fr: 'Andromède' },
}

I don’t have a strong opinion — we use the string | Record<string, string> form for the site title, but the label/translations form here in the sidebar currently. I’m pretty sure I opted for the separate translations property when designing the sidebar API to enforce the presence of a default label at the schema level, but having translations keys proliferating everywhere also isn’t amazing.

Happy to keep this as is, but wanted to note this while approving!

@delucis delucis added the 🌟 minor Change that triggers a minor release label Sep 11, 2024
@HiDeoo
Copy link
Member Author

HiDeoo commented Sep 11, 2024

Updated the test coverage thresholds in fd48b6b (no other changes since the approval)

@delucis delucis merged commit 7286220 into withastro:main Sep 18, 2024
16 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Sep 18, 2024
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 🌟 minor Change that triggers a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants