fix: add portal toggle to Popover and i18n for Combobox remove - #1131
fix: add portal toggle to Popover and i18n for Combobox remove#1131kittyyueli wants to merge 2 commits into
Conversation
The badge's remove button hard-coded "Remove" for screen readers while the item already declared react-i18next as a dependency. Route it through t(), the same way sheet.tsx handles its close label, and add the key to en.json. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SnmiT3edq81s2qTDqoE8zP
Portaled content sits outside an enclosing dialog's focus scope, which then
reclaims focus from it and leaves nested inputs untypeable. portal={false}
keeps the content in the tree; the default is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SnmiT3edq81s2qTDqoE8zP
kittyyueli
changed the title
|
Apollo Coded App preview deployments are ready.
|
Dependency License Review
License distribution
Excluded packages
|
|
The It reproduces across unrelated PRs: runs 1120, 1121, 1122, 1123, 1124, 1125, 1126–1130, 1131 and 1132 all failed the same way, on five different branches and four authors. The only recent pass is 1133, the localization sync bot. That evidence is stronger than a re-run would be, so I have not spent one — a re-run fails identically while the permission is missing. No fix exists that belongs in this PR. The two real fixes are both outside it:
I have deliberately not done the second: editing anything outside Every other check is green or still running; this is the only failure on Generated by Claude Code |
There was a problem hiding this comment.
🟡 Changes recommended
The new remove i18n key is only added to en.json, leaving other supported locale files out of sync and potentially untranslated in non-English locales.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the Apollo Vertex registry UI components to (1) allow Popover content to render without a Radix Portal when needed (e.g., inside dialogs with focus management), and (2) internationalize the Combobox badge “Remove” screen-reader label via react-i18next.
Changes:
- Added an optional
portal?: booleanprop toPopoverContent(defaulttrue) to conditionally wrap content inPopoverPrimitive.Portal. - Replaced the hardcoded
"Remove"sr-only label inComboboxBadgewitht("remove"). - Added an English
removetranslation key.
File summaries
| File | Description |
|---|---|
| apps/apollo-vertex/registry/popover/popover.tsx | Adds a portal toggle to render popover content in-place when needed for focus-scoped containers (e.g., dialogs). |
| apps/apollo-vertex/registry/combobox/combobox.tsx | Uses i18n (useTranslation) for the Combobox badge remove button sr-only label. |
| apps/apollo-vertex/locales/en.json | Adds the remove translation key in English. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "projects": "Projects", | ||
| "recent_activity": "Recent Activity", | ||
| "recent_invoices": "Recent Invoices", | ||
| "remove": "Remove", | ||
| "remove_file": "Remove {{name}}", | ||
| "remove_from_expected_result": "Remove from baseline", |
There was a problem hiding this comment.
Checked this, and I'm leaving it as-is — English-only is the required state for a new key in this repo.
apps/apollo-vertex/AGENTS.md:
Only edit
locales/en.json. Add new keys there in alphabetical order. Do not touch the other locale files — the localization team syncs translations into them as a separate workflow. PRs that modify non-English locales will be sent back.
On the two predicted outcomes: lib/i18n.ts sets fallbackLng: DEFAULT_LOCALE with DEFAULT_LOCALE = "en", so a missing key renders the English string. The raw key is never shown.
There is precedent in the tree too — 13 keys in en.json are absent from de.json right now, the 12 confidence_signal_* keys plus this one, all waiting on the same sync. #1126 is an example of that sync landing.
"remove" was inserted alphabetically, immediately before "remove_file".
Generated by Claude Code
📊 Coverage + size by packagePer-package bundle size on this PR (no JS/TS source changes detected under
"Coverage" is each package's own |
Storybook visual diff✅ No stories are affected by this PR's changes; nothing to compare. Logs Updated (PT): Sep 07, 2026, 09:50:22 PM |
Summary
This PR makes two targeted improvements to the component library:
Changes
Popover (
popover.tsx)portalprop (defaults totrue) toPopoverContentto conditionally wrap content inPopoverPrimitive.Portalportal={false}keeps it inside so nested inputs remain usableReact.Fragmentas wrapper whenportal={false}Combobox (
combobox.tsx)useTranslationfromreact-i18nextComboboxBadgeto uset("remove")instead of hardcoded "Remove" stringsr-onlyclass for screen reader accessibilityLocalization (
en.json)"remove": "Remove"entry to English locale fileImplementation Details
The Popover change is backward-compatible—the default
portal={true}preserves existing behavior. The conditional wrapper pattern allows consumers to opt into keeping popover content within a dialog's focus scope when needed.The Combobox i18n change follows the existing translation pattern in the codebase and enables multi-language support for the remove button label.
https://claude.ai/code/session_01SnmiT3edq81s2qTDqoE8zP