Skip to content

Missing translation strings#1690

Merged
bookernath merged 4 commits intobigcommerce:mainfrom
thebigrick:missing-localization
Nov 29, 2024
Merged

Missing translation strings#1690
bookernath merged 4 commits intobigcommerce:mainfrom
thebigrick:missing-localization

Conversation

@thebigrick
Copy link
Copy Markdown
Contributor

What/Why?

Some of the aria-label attributes and a few strings were hardcoded.
I added new entries to the en.json file and applied the translations in the TSX files.

Testing

Strings should yet be there. And should be translatable.

@thebigrick thebigrick requested a review from a team November 28, 2024 20:01
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Nov 28, 2024

🦋 Changeset detected

Latest commit: b77f2da

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

This PR includes changesets to release 1 package
Name Type
@bigcommerce/catalyst-core Patch

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

@vercel
Copy link
Copy Markdown

vercel Bot commented Nov 28, 2024

@thebigrick is attempting to deploy a commit to the BigCommerce Platform Team on Vercel.

A member of the Team first needs to authorize it.

@bookernath
Copy link
Copy Markdown
Contributor

Thanks @thebigrick!

@vercel
Copy link
Copy Markdown

vercel Bot commented Nov 29, 2024

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

Name Status Preview Comments Updated (UTC)
catalyst-latest ✅ Ready (Inspect) Visit Preview Nov 29, 2024 3:16pm
catalyst-soul ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2024 3:16pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
catalyst-au ⬜️ Ignored (Inspect) Visit Preview Nov 29, 2024 3:16pm
catalyst-uk ⬜️ Ignored (Inspect) Visit Preview Nov 29, 2024 3:16pm

@thebigrick
Copy link
Copy Markdown
Contributor Author

I just fixed a typo in the en.json file causing a build failure

@github-actions
Copy link
Copy Markdown
Contributor

⚡️🏠 Lighthouse report

Lighthouse ran against https://catalyst-latest-9eop7vixt-bigcommerce-platform.vercel.app

🖥️ Desktop

We ran Lighthouse against the changes on a desktop and produced this report. Here's the summary:

Category Score
🟠 Performance 77
🟢 Accessibility 96
🟢 Best practices 100
🟠 SEO 82

📱 Mobile

We ran Lighthouse against the changes on a mobile and produced this report. Here's the summary:

Category Score
🟠 Performance 87
🟢 Accessibility 96
🟢 Best practices 100
🟠 SEO 85

Copy link
Copy Markdown
Contributor

@chanceaclark chanceaclark left a comment

Choose a reason for hiding this comment

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

Thanks for updating these! I know you already merged, but left some comments if you want to go back and clean some of them up.

Comment on lines 34 to +35
const t = await getTranslations('NotFound');
const ct = await getTranslations('Components.Header.MiniCart');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would love to know if there are performance implications doing this double await for translations.

Also, I think our pattern for things like this is just throw another translation string in the NotFound namespace instead of creating a new translation getter.


if (!count) {
return <ShoppingCart aria-label="cart" />;
return <ShoppingCart aria-label={t('cart')} />;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know you were just updating the current strings, but going off the rules of aria we should only use aria- roles if it's absolutely necessary. We should be using the <title> element for SVGs instead:

<ShoppingCart>
  <title>{t('cart')}</title>
</ShoppingCart>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants