fix(search): ExpandableSearch: collapsed-state tooltip label is hardcoded "Search" with no way to override it - #23080
Conversation
✅ Deploy Preview for v12-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v12-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
f425dcc to
bd3ad14
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23080 +/- ##
==========================================
+ Coverage 89.80% 90.49% +0.68%
==========================================
Files 626 271 -355
Lines 55608 40643 -14965
Branches 7954 2234 -5720
==========================================
- Hits 49940 36780 -13160
+ Misses 5522 3845 -1677
+ Partials 146 18 -128
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…oded "Search" with no way to override it
bd3ad14 to
fbf975c
Compare
| /** | ||
| * Specify the label for the expand button in ExpandableSearch (collapsed state tooltip). | ||
| * Use this prop to provide a localised string in place of the default "Search" label. | ||
| */ | ||
| expandButtonLabelText?: string; |
There was a problem hiding this comment.
Did you explore using the existing translateWithId pattern for this internal tooltip string instead of adding a new expandButtonLabelText prop? Other components (e.g., ProgressIndicator, DataTable) use translation IDs for built in labels, which would support localization without adding another component level label prop. If not, I'd suggest considering this approach.
There was a problem hiding this comment.
I noticed that Search component is using an individual prop closeButtonLabelText, so I followed this pattern.
On the other hand,ListBoxMenuIcon (used by e.g. ComboBox) uses translateWithId with close.menu / open.menu IDs.
Introducing translateWithId here alongside closeButtonLabelText feels inconsistent within the same component - should closeButtonLabelText be deprecated in favour of translateWithId as well? Happy to go either way, just want to keep it consistent.
There was a problem hiding this comment.
should
closeButtonLabelTextbe deprecated in favour oftranslateWithIdas well?
Sounds like the right move to me. I'm not sure if others have different opinions.
There was a problem hiding this comment.
- Followed the
required v12 migration documentation
for any code change that affects v12, or struck through this item because
the PR does not affect v12
The v12 migration checklist item is checked, but I don't see a corresponding update to docs/migration/v12.md. Am I missing something?
@adamalston you're right, sorry for the confusion - I checked that by mistake instead of striking it through. I've updated the checklist. |
2 participants
Closes #23076
Add
expandButtonLabelTextprop toSearch/ExpandableSearchto allow localising the collapsed-state tooltip label.Changelog
New
Search/ExpandableSearch: addedexpandButtonLabelTextprop (default: "Search") to override the tooltip shown over the magnifier button when collapsedTesting / Reviewing
<ExpandableSearch expandButtonLabelText="Suche" />, hover over the magnifier button while collapsed and confirm the tooltip reads "Suche" instead of "Search"yarn test --testPathPatterns="Search-test"- all 73 tests passyarn test --testPathPatterns="ExpandableSearch-test"- all 17 tests passPR Checklist
As the author of this PR, before marking ready for review, confirm you:
Followed therequired v12 migration documentation
for any code change that affects v12, or struck through this item because
the PR does not affect v12
More details can be found in the pull request guide