fix: Support unsetting the text resource in the text resource picker #14496
Conversation
|
Warning Rate limit exceeded@standeren has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces changes to the text resource handling in the frontend components, specifically focusing on adding an "unset option" label and removing the empty resource list text. The modifications span multiple files across the studio components, affecting how text resources are displayed and managed. The changes primarily involve updating type definitions, test files, and component implementations to support a new way of representing unset or empty options. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
area/ui-editor
TomasEng
force-pushed
the
suppurt-unset-text-resource
branch
from
4c63325 to
4056bcb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14496 +/- ##
=======================================
Coverage 95.70% 95.70%
=======================================
Files 1901 1901
Lines 24730 24733 +3
Branches 2831 2831
=======================================
+ Hits 23667 23670 +3
Misses 802 802
Partials 261 261 ☔ View full report in Codecov by Sentry. |
TomasEng
force-pushed
the
suppurt-unset-text-resource
branch
from
4056bcb to
5b32fcf
Compare
TomasEng
added
team/studio-domain1
skip-releasenotes
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.stories.tsx (1)
18-18: Consider using i18n keys in Storybook stories.While the Norwegian text "Ikke oppgitt" works, consider using translation keys for consistency across the application. This would make the stories more maintainable and aligned with the application's internationalization approach.
frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.stories.tsx (1)
43-43: Maintain consistency with StudioTextResourcePicker stories.The implementation looks good, but consider standardizing the approach to internationalization across all component stories. This could involve either:
- Using translation keys consistently across all stories
- Using English placeholder text for all stories
frontend/libs/studio-components/src/components/StudioInputTable/test-data/testTableData.ts (1)
41-41: Consider using English in test data.The unset option label is in Norwegian ('Ikke oppgitt'). Consider using English for test data consistency unless specifically testing localization.
- unsetOptionLabel: 'Ikke oppgitt', + unsetOptionLabel: 'Not specified',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
frontend/language/src/nb.json(1 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/texts.ts(1 hunks)frontend/libs/studio-components/src/components/StudioInputTable/test-data/testTableData.ts(2 hunks)frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.stories.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.test.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioTextResourceInput/types/Mode.ts(1 hunks)frontend/libs/studio-components/src/components/StudioTextResourceInput/types/TextResourceInputTexts.ts(1 hunks)frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.module.css(1 hunks)frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.stories.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.test.tsx(2 hunks)frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.tsx(1 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/hooks/useCodeListEditorTexts.ts(1 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditOptions/OptionTabs/hooks/useOptionListEditorTexts.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.module.css
- frontend/libs/studio-components/src/components/StudioTextResourceInput/types/Mode.ts
🔇 Additional comments (15)
frontend/libs/studio-components/src/components/StudioTextResourceInput/types/TextResourceInputTexts.ts (1)
7-7: LGTM! Type definition updated to support unsetting text resources.The addition of
unsetOptionLabeland removal ofemptyResourceListaligns well with the PR objective to support undefined text resources.frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/hooks/useCodeListEditorTexts.ts (1)
45-45: LGTM! Proper implementation of i18n for the unset option.The implementation correctly uses the translation system and maintains type safety. The translation key structure follows the established pattern.
frontend/packages/ux-editor/src/components/config/editModal/EditOptions/OptionTabs/hooks/useOptionListEditorTexts.ts (1)
46-46: LGTM! Clean addition of unset option label.The addition of
unsetOptionLabelfollows the existing translation pattern and integrates well with the i18n system.frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.tsx (4)
11-14: LGTM! Type changes align with feature requirements.The updated type signature for
onValueChangeand addition of requiredunsetOptionLabelprop properly support the unsetting feature.
41-48: Good documentation of component structure requirement.The comment explaining why this can't be a component function is valuable for maintainability.
29-31: Consider handling undefined value case.The current implementation handles null/empty values by defaulting to
['']. Consider if undefined should be handled differently.- value={value ? [value] : ['']} + value={value != null ? [value] : ['']}
20-24: Verify the null handling behavior.The
handleValueChangecallback converts empty string to null. Ensure this behavior is consistent with other form components in the codebase.✅ Verification successful
Empty string to null conversion is correct and consistent
The null handling in
StudioTextResourcePickeris intentional and well-tested. The conversion of empty string to null aligns with the component's unset option behavior, which is explicitly verified in the test suite. This pattern ensures consistent handling of unset values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar form value handling patterns rg -l "onValueChange.*null" frontend/libs/studio-components/src/components/Length of output: 284
Script:
#!/bin/bash # Search for form components and their value handling rg -l "onChange.*value" frontend/libs/studio-components/src/components/ # Get the content of the test file to understand expected behavior cat frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.test.tsx # Use ast-grep to find similar array destructuring patterns in value handling ast-grep --pattern 'const $_ = useCallback(($_: $_[]) => $_($_), [$_])'Length of output: 9862
Script:
#!/bin/bash # Check implementation of similar components rg -A 3 "onValueChange.*null" frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.tsx rg -A 3 "onValueChange.*null" frontend/libs/studio-components/src/components/StudioStatusRadioGroup/StudioStatusRadioGroup.tsx # Look for similar picker/select components fd "Studio.*Picker.tsx|Studio.*Select.tsx" frontend/libs/studio-components/src/components/Length of output: 542
Script:
#!/bin/bash # Check StudioNativeSelect implementation cat frontend/libs/studio-components/src/components/StudioNativeSelect/StudioNativeSelect.tsx # Look for its tests as well cat frontend/libs/studio-components/src/components/StudioNativeSelect/StudioNativeSelect.test.tsxLength of output: 1054
frontend/libs/studio-components/src/components/StudioInputTable/test-data/testTableData.ts (1)
2-2: LGTM! Import path updated correctly.The import path change reflects proper module organization.
frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/texts.ts (1)
37-37: LGTM! Test data properly updated.The addition of
unsetOptionLabelwith English text 'None' is consistent with test data best practices.frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.test.tsx (3)
17-17: LGTM! Good test setup.The addition of
unsetOptionLabelto defaultProps ensures consistent test behavior.Also applies to: 21-21
25-25: LGTM! Good test hygiene.Adding
beforeEach(jest.clearAllMocks)ensures clean test state between runs.
69-90: LGTM! Comprehensive test coverage for the new unset feature.The new test cases thoroughly verify the unset option functionality:
- Displays the unset option
- Shows unset option as default selection
- Triggers callback with null when unset is selected
frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.test.tsx (1)
24-24: LGTM! Good localization.Added Norwegian translation 'Ikke oppgitt' for the unset option label.
frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.tsx (1)
135-136: LGTM! Clean prop passing.Good prop organization and proper forwarding of
unsetOptionLabelto StudioTextResourcePicker.frontend/language/src/nb.json (1)
160-160: LGTM! Consistent localization.Added Norwegian translation key and value following the established naming pattern.
.../studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.test.tsx
Outdated
Show resolved
Hide resolved
.../studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.test.tsx
Show resolved
Hide resolved
.../libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.tsx
Show resolved
Hide resolved
Description
In order to support undefined text resources in code lists, we need to support it in the text resource picker. This pull request adds an option for unsetting the text resource. Since this option is always present, the option list is never empty, som I have also removed the use of the
StudioCombobox.Emptycomponent.The updated component calls the
onChangeValuecallback withnullwhen the user chooses to unset the text resource.I have added the
skip-manual-testinglabel since it makes more sense to test it together with my next pull request, which will extend this functionality to theTextResourceInputcomponent.Related Issue
Verification
Summary by CodeRabbit
New Features
Style
Refactor
Bug Fixes