-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Make label required #14826
fix: Make label required #14826
Conversation
📝 WalkthroughWalkthroughThis pull request introduces various updates to improve text resource handling. A new localization entry for empty text resources is added to the Norwegian language JSON file. Several components and their type definitions are extended to include a new optional or required property for text resource inputs. In addition, test cases, test data, story definitions, and hook functions are updated to incorporate an Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14826 +/- ##
=======================================
Coverage 95.79% 95.79%
=======================================
Files 1924 1924
Lines 25107 25107
Branches 2869 2869
=======================================
Hits 24050 24050
Misses 797 797
Partials 260 260 ☔ View full report in Codecov by Sentry. |
# Conflicts: # frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditor.test.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/editModal/EditOptions/OptionTabs/hooks/useOptionListEditorTexts.ts (1)
47-50
: New property added alongside existing similar propertyThe addition of
emptyTextResourceList
provides a dedicated property for empty text resource messaging. However, I notice it uses the same translation key as the existingemptyResourceList
property on the next line. Consider if maintaining both properties with identical values might cause confusion for future maintainers.If both properties are intentionally maintained for separate use cases, consider adding a code comment explaining the distinction between them to improve code maintainability.
frontend/language/src/nb.json (1)
175-175
: Potential Typo in Localization EntryThe new key
"code_list_editor.text_resource.empty_list"
has been added with the value"Ingen tekstressurser er tigjengelige"
. It appears that the word "tigjengelige" may be a typo and should likely be corrected to "tilgjengelige" to ensure proper Norwegian spelling.- "code_list_editor.text_resource.empty_list": "Ingen tekstressurser er tigjengelige", + "code_list_editor.text_resource.empty_list": "Ingen tekstressurser er tilgjengelige",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/language/src/nb.json
(1 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditor.test.tsx
(2 hunks)frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx
(6 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
(1 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
(3 hunks)frontend/libs/studio-components/src/components/StudioTextResourceInput/types/TextResourceInputTexts.ts
(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)
🧰 Additional context used
🧠 Learnings (1)
frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.tsx (1)
Learnt from: TomasEng
PR: Altinn/altinn-studio#14646
File: frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.tsx:30-30
Timestamp: 2025-02-13T06:29:43.924Z
Learning: TypeScript's `strictNullChecks` mode helps catch potential null/undefined errors at compile time, making it easier to identify issues with optional properties and callbacks.
🔇 Additional comments (18)
frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.stories.tsx (1)
39-46
: Added property for empty text resource list message in story argsThe addition of the
emptyTextResourceList
property in the texts object properly aligns with the PR objectives, providing a user-friendly message when no text resources are available. This change ensures consistent messaging throughout the application.frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/hooks/useCodeListEditorTexts.ts (1)
48-48
: Good addition for handling empty text resource lists!Adding the
emptyTextResourceList
property improves user experience by providing a message when no text resources are available. The implementation correctly uses the translation key structure.frontend/libs/studio-components/src/components/StudioCodelistEditor/test-data/texts.ts (1)
49-49
: Test data updated to match the new interface requirements.The addition of
emptyTextResourceList
to the test data ensures consistency with the updated component interface. This will help maintain test coverage for scenarios with empty text resource lists.frontend/libs/studio-components/src/components/StudioTextResourceInput/types/TextResourceInputTexts.ts (1)
7-8
: Updated type definition with appropriate optionality.The interface changes align well with the PR objective:
- Adding the optional
emptyTextResourceList
property provides a way to display messages when no text resources are available- Making
noTextResourceOptionLabel
optional allows for more flexibility in component usageThese changes improve the component's adaptability while maintaining backward compatibility.
frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.test.tsx (1)
20-20
: Test data updated with Norwegian localization for empty text resources.Good addition of the
emptyTextResourceList
property with the Norwegian text "Ingen tekstressurser er tilgjengelige". This ensures that tests properly verify the component's behavior when dealing with empty text resource lists.frontend/libs/studio-components/src/components/StudioTextResourceInput/StudioTextResourceInput.tsx (3)
31-31
: Added required property to support validationThis addition of the optional
required
property to theTextResourceInputPropsBase
type allows the component to support input validation, aligning with the PR objective to make labels required.
107-107
: Good addition of required property to destructured propsProperly destructuring the required property from the props object to make it available within the InputBox component.
138-138
: Correctly passing required property to StudioTextResourcePickerPassing the required property to the StudioTextResourcePicker enables validation at the picker level, ensuring it can properly communicate the required state to users.
frontend/libs/studio-components/src/components/StudioInputTable/test-data/testTableData.ts (1)
38-38
: Added feedback message for empty text resourcesAdded a clear user feedback message for scenarios when no text resources are available, improving the user experience.
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsx (6)
83-83
: Correctly marking label field as requiredSetting the label field as required aligns with the PR objective to make labels mandatory, ensuring users provide this important information.
93-93
: Appropriately marking description as optionalCorrectly setting the description field as optional, maintaining flexibility for users while enforcing requirements only where necessary.
103-103
: Appropriately marking help text as optionalCorrectly setting the help text field as optional, consistent with the approach used for the description field.
219-219
: Added required property to TextResourceIdCellProps typeAdding the required property to the props type ensures type safety and proper documentation of the component API.
238-238
: Properly receiving required prop in TextResourceSelectorCellCorrectly destructuring the required prop from the component properties.
249-249
: Properly passing required prop to child componentCorrectly passing the required property down to the StudioInputTable.Cell.TextResource component, ensuring the validation requirement is propagated through the component hierarchy.
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditor.test.tsx (3)
44-47
: Good test setup for text resourcesCreated a reusable props object with text resources for testing, improving test organization and maintainability.
102-111
: Test ensuring labels cannot be unsetGreat test that verifies the required behavior for labels - confirming that the "unset" option is not available for required fields.
113-125
: Comprehensive testing of optional fieldsExcellent parameterized test that verifies both description and help text fields (which are optional) correctly display the "unset" option. This approach reduces test duplication and ensures both optional properties are properly tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but one small comment on language typo 😄
Co-authored-by: William Thorenfeldt <48119543+wrt95@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ok, good work!
Description
This pull request makes the label field of the code list editor required, enfording the user to select a text resource for it.
Spiller.inn.2025-02-26.151912.mp4
Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Tests
Refactor