fix(dashboard): Localize configurable operation selector buttons#4914
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (31)
📝 Walkthrough
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
There was a problem hiding this comment.
Reviewed before I spotted that master had already fixed part of this — see my follow-up comment for the full picture. Superseding this review body, and I've deleted the inline suggestions since they assumed the ReactNode shape.
Two points survive regardless of which shape you land:
- The
@exampleblocks show raw strings (configurable-operation-selector.tsx:69,configurable-operation-multi-selector.tsx:83-84,95). Whatever the final API is, the examples should demonstrate the localized form — they're the copy-paste source for extension authors, and a raw string type-checks while silently never translating. - The prop doc comments say "Text to display" (
configurable-operation-selector.tsx:32,34, multi-selector:34,36,38). Worth a pass once the prop types settle.
Otherwise: root cause is correctly diagnosed, call sites are all covered, and the catalogs match lingui extract.
|
(Edited: my original wording claimed master's fix "works by accident" via a failed catalog lookup. That's wrong — corrected below.) Heads up — #4410 is genuinely fixed on master. #: src/lib/components/shared/configurable-operation-multi-selector.tsx:265
#: src/lib/components/shared/configurable-operation-selector.tsx:140
msgid "{buttonText}"
msgstr "{buttonText}"It renders the prop verbatim in every locale. Where the caller passes Still worth landing this, for three reasons:
Suggested reshape — rebase, then do only what master didn't:
That lets you drop the Diff shrinks to ~15 lines, and most I've deleted my inline suggestions above — they assumed the |
The `<Trans>{buttonText}</Trans>` in the two configurable operation
selectors extracted to an identity passthrough (`msgid "{buttonText}"`,
`msgstr "{buttonText}"`), so it rendered the prop verbatim in every
locale. Callers that passed a pre-translated string via the `t` macro
worked; callers that passed a raw string silently shipped English.
The promotion conditions and actions selectors did the latter, so their
"Add condition" / "Add action" buttons and dropdown titles never
translated. `emptyText` had the same problem via its default value.
Remove the no-op `<Trans>` wrapper, translate the promotion call sites
with the `t` macro (matching the other five call sites), and translate
the `emptyText` default at render.
Relates to vendurehq#4410
Co-authored-by: Martin Grolmus <martin@stagro.sk>
biggamesmallworld
force-pushed
the
mgrolmus/oss-405-dashboard-i18n-regression-collections-add-collection-filter
branch
from
574055e to
27ef613
Compare
|
Took you up on the offer — force-pushed a reshaped commit over What changed vs. your version:
Result: 4 source files, ~30 lines, +5 msgids across the catalogs. Conflicts resolved. Retitled in the commit since it's no longer about collection filters — master's Still open as follow-ups, not touched here: Thanks for the original diagnosis — the root cause writeup is what made this quick. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
grolmus
left a comment
grolmus
left a comment
There was a problem hiding this comment.
Thanks @biggamesmallworld — this is a clean fix. <Trans>{buttonText}</Trans> was the classic lingui anti-pattern (the macro can't extract a runtime variable, so those button/dropdown strings were never actually translated). Moving the translation to the static call sites with the t macro, rendering the passed string plainly, and translating the internal emptyText fallback via useLingui is exactly the right approach. Good call documenting "must be pre-translated" on the buttonText prop too.
I checked it out and verified:
- ✅ Ran the exact CI check (
scripts/check-i18n-sync.sh) → i18n catalogs are in sync, solingui extractproduces no drift. - ✅ No leftover
<Trans>references after the import swap in either shared selector — clean. - ✅ All configurable-operation selector call sites are localized: the two promotion selectors here, plus the payment/shipping/collection selectors which were already migrated on
master— so coverage is complete, not just promotions. - ✅
dropdownTitleis rendered plainly, consistent with it now being passed pre-translated.
No regression for any caller (the old <Trans>{var}</Trans> didn't translate either), and the anti-pattern is gone repo-wide for these components. LGTM 🚀
grolmus
requested review from
BibiSebi and
michaelbromley
and removed request for
BibiSebi and
michaelbromley
3 participants
Summary
Fixes a Dashboard i18n regression where the "Add collection filter" button — and the equivalent buttons on promotion conditions/actions and the payment/shipping method selectors — stayed in English after switching the UI language.
Root cause
The two shared selector components rendered the button/empty labels as:
buttonTextis a runtime string prop. Lingui's<Trans>macro only extracts static literals at compile time, so wrapping a variable produces a placeholder message that is never translated — the raw English prop was rendered regardless of locale.Change
configurable-operation-multi-selector.tsx/configurable-operation-selector.tsx:buttonText/dropdownTitle/emptyTextprop types →ReactNode; render{buttonText}directly; defaultemptyText→<Trans>No options found</Trans>.<Trans>…</Trans>elements, which extract correctly.lingui extractand translated the 11 new message ids across all 26 locale catalogs.Test plan
Automated
tsc(tsconfig.check.json) passes.lingui extractis idempotent → thecheck-i18n-syncCI gate stays green.Manual (screenshots on the Linear issue / attached to this PR)
/collections/new) via the e2e harness and switched UI language:es→ button renders "Añadir filtro de colección"de→ button renders "Sammlungsfilter hinzufügen"es.Notes / out of scope
ConfigurableOperationDef.description/arg.label) and are not addressed here — that is the separate configurable-operation server-translation gap.product-multi-selector-input.tsxhas the same<Trans>{buttonText}>anti-pattern but in a template-literal-with-interpolation form; left as a follow-up.Fixes #4410
Manual-verification screenshots (en/es/de) are attached on the Linear issue OSS-405.