feat: reorganize summary2 config #14460
Conversation
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the frontend's user interface and localization, focusing on improving component configuration and error messaging. The changes include adding a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (2)
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
Jondyr
changed the base branch from
main
to
08-01-summary2-overrides-design-update
Jondyr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx (1)
65-121: Consider extracting repeated component patterns.The component has repeated patterns for different target types (page, component, layoutSet) with similar props and structure. Consider extracting these into a separate component to reduce duplication and improve maintainability.
Example refactor:
type TargetSelectorProps = { type: 'page' | 'component' | 'layoutSet'; value: string; options?: Array<{ id: string; label: string }>; onValueChange?: (value: string) => void; label: string; }; const TargetSelector = ({ type, value, options, onValueChange, label }: TargetSelectorProps) => { if (type === 'layoutSet') { return ( <StudioTextfield key={value} size='sm' label={label} value={value} disabled={true} /> ); } return ( <Summary2ComponentReferenceSelector key={value} label={label} value={value} options={options} onValueChange={onValueChange} /> ); };frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
99-99: Usetype='button'for non-form buttons to prevent unintended form submissionsThe
StudioButtonon line 99 hastype='submit', which may trigger unintended form submissions if this component is placed within a<form>element. It's recommended to settype='button'for buttons that do not submit a form.Apply this diff to change the button type:
- type='submit' + type='button'frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx (1)
1-26: Align component name with filenameThe component name
OverrideShowComponentSwitchdoesn't match the filenameForceShowSwitch.tsx. Additionally, the props interfaceForceShowSwitchPropsdoesn't match the component name. Consider renaming for consistency:
- Either rename the file to match the component:
OverrideShowComponentSwitch.tsx- Or rename the component to match the file:
ForceShowSwitchfrontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx (1)
17-24: Remove unnecessary async handlerThe
onChangehandler is marked as async but doesn't contain any asynchronous operations. Consider removing the async keyword:- onChange={async (event: React.ChangeEvent<HTMLInputElement>) => { + onChange={(event: React.ChangeEvent<HTMLInputElement>) => {frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx (1)
24-31: Remove redundant icon propThe
iconprop is set to an empty string which is redundant. Consider removing it since it doesn't serve any purpose:inputProps={{ - icon: '', label: t('ux_editor.component_properties.summary.override.empty_field_text'), size: 'sm', value: override.emptyFieldText ?? '', onChange: (event: React.ChangeEvent<HTMLInputElement>) => onChange({ ...override, emptyFieldText: event.target.value }), }}frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx (2)
22-24: Optimize layout components retrievalThe flatMap operation could be simplified using Object.values:
- const components = Object.values(formLayoutsData).flatMap((layout) => - getAllLayoutComponents(layout), - ); + const components = Object.values(formLayoutsData).flatMap(getAllLayoutComponents);
26-26: Remove redundant type castingThe type casting to ComponentType is redundant:
- const isGroupComponent = component?.type === (ComponentType.Group as ComponentType); + const isGroupComponent = component?.type === ComponentType.Group;frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
50-55: Consider simplifying the open state toggle logic.The current implementation could be more concise.
- setOpen={(open) => - open - ? setOpenOverrides([...openOverrides, index]) - : setOpenOverrides(openOverrides.filter((i) => i !== index)) - } + setOpen={(open) => + setOpenOverrides(open + ? [...openOverrides, index] + : openOverrides.filter((i) => i !== index)) + }frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx (1)
25-25: Consider simplifying accordion state.Since there's only one accordion section, using a Record might be unnecessary.
- const [accordionOpen, setAccordionOpen] = React.useState<Record<string, boolean>>({}); + const [isAccordionOpen, setIsAccordionOpen] = React.useState(false);frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.test.tsx (2)
14-19: LGTM! Consider adding validation for the summary2Component mock.The mock setup looks good but could be enhanced with additional properties to test edge cases.
const summary2Component: FormItem = { id: '0', type: ComponentType.Summary2, itemType: 'COMPONENT', target: {}, + // Add validation for required properties + required: true, + readOnly: false, + hidden: false, };
21-49: Consider adding more test coverage for edge cases.The test suite covers basic functionality but could benefit from additional test cases:
- Error handling scenarios
- Invalid override configurations
- Empty overrides array
- Maximum number of overrides
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
41-42: Fix typo in test description and improve readability.The test description has a typo in "componenetId" and could be more descriptive.
- it('should be able to show "vis type" comobox when componenetId is checkbox', async () => { + it('should show display type combobox when component is Checkboxes', async () => {Also applies to: 43-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
frontend/language/src/nb.json(2 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx(2 hunks)frontend/packages/ux-editor/src/components/RequiredIndicator.module.css(1 hunks)frontend/packages/ux-editor/src/components/RequiredIndicator.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx(16 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx(3 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/hook/useCustomConfigType.ts(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/index.ts(0 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2ComponentReferenceSelector.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/index.ts
✅ Files skipped from review due to trivial changes (5)
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.module.css
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.module.css
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2ComponentReferenceSelector.tsx
- frontend/packages/ux-editor/src/components/RequiredIndicator.module.css
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.module.css
🧰 Additional context used
📓 Learnings (6)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx (1)
Learnt from: Jondyr
PR: Altinn/altinn-studio#14379
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx:17-24
Timestamp: 2025-01-13T12:46:32.116Z
Learning: In the Summary2 component's override configuration, the `hideEmptyFields` and `forceShow` properties are intentionally controlled by the same switch, where `hideEmptyFields` is set to the inverse of `forceShow`.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx (2)
Learnt from: Jondyr
PR: Altinn/altinn-studio#14379
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx:17-24
Timestamp: 2025-01-13T12:46:32.116Z
Learning: In the Summary2 component's override configuration, the `hideEmptyFields` and `forceShow` properties are intentionally controlled by the same switch, where `hideEmptyFields` is set to the inverse of `forceShow`.
Learnt from: Jondyr
PR: Altinn/altinn-studio#14379
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx:63-70
Timestamp: 2025-01-13T12:44:45.751Z
Learning: In the Altinn Studio codebase, when using StudioProperty.Button component, it's expected to pass `false` as the value prop when no meaningful value is available (e.g., when componentNameType is undefined). This is the intended behavior and should not be changed to handle undefined cases differently.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx (1)
Learnt from: Jondyr
PR: Altinn/altinn-studio#14379
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx:17-24
Timestamp: 2025-01-13T12:46:32.116Z
Learning: In the Summary2 component's override configuration, the `hideEmptyFields` and `forceShow` properties are intentionally controlled by the same switch, where `hideEmptyFields` is set to the inverse of `forceShow`.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
Learnt from: Jondyr
PR: Altinn/altinn-studio#14379
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx:63-70
Timestamp: 2025-01-13T12:44:45.751Z
Learning: In the Altinn Studio codebase, when using StudioProperty.Button component, it's expected to pass `false` as the value prop when no meaningful value is available (e.g., when componentNameType is undefined). This is the intended behavior and should not be changed to handle undefined cases differently.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx (1)
Learnt from: Jondyr
PR: Altinn/altinn-studio#14379
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx:17-24
Timestamp: 2025-01-13T12:46:32.116Z
Learning: In the Summary2 component's override configuration, the `hideEmptyFields` and `forceShow` properties are intentionally controlled by the same switch, where `hideEmptyFields` is set to the inverse of `forceShow`.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
Learnt from: Jondyr
PR: Altinn/altinn-studio#14379
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx:63-70
Timestamp: 2025-01-13T12:44:45.751Z
Learning: In the Altinn Studio codebase, when using StudioProperty.Button component, it's expected to pass `false` as the value prop when no meaningful value is available (e.g., when componentNameType is undefined). This is the intended behavior and should not be changed to handle undefined cases differently.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Testing
🔇 Additional comments (13)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx (2)
1-1: LGTM! Import statements are well-organized.The typo fix in the import statement and the addition of CSS modules improve code organization and maintainability.
Also applies to: 9-9
Line range hint
26-64: Well-structured component implementation!The component demonstrates good practices:
- Proper use of React hooks
- Type-safe event handlers
- Clean state management
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
65-65: Consistent handling ofvalueprop aligns with codebase conventionsThe
valueprop inStudioProperty.Buttoncorrectly passesfalsewhencomponentNameTypeis undefined. This adheres to the intended behavior in the codebase, as noted in the retrieved learnings.frontend/packages/ux-editor/src/components/RequiredIndicator.tsx (1)
1-14:RequiredIndicatorcomponent is well implementedThe
RequiredIndicatorcomponent correctly utilizes translation and styling, enhancing user feedback for required fields.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/hook/useCustomConfigType.ts (1)
13-21: Proper use of translation keys improves internationalizationThe labels for the custom config types now correctly use the
tfunction for translations, enhancing internationalization support.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (2)
29-35: LGTM! Clean implementation of conditional rendering.The early return pattern is well implemented, making the code more maintainable and easier to understand.
41-41: LGTM! Improved translation key specificity.The translation key change from 'overrides_type' to 'summary.override.display_type' provides better context and maintainability.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx (1)
53-53: LGTM! Clean integration of ComponentMainConfig.The placement and prop passing align well with the PR objective of moving summary2 configuration to the main level.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
37-39: LGTM! Clean implementation of state management.The state update logic in
onDeleteOverridecorrectly handles index adjustments when removing items.frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx (1)
28-38: LGTM! Clean implementation of update handlers.The handlers properly maintain immutability and type safety.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (2)
349-357: LGTM! Good test for hideEmptyFields info message.The test case properly validates the display of info message when hideEmptyFields is true.
359-373: LGTM! Good test for collapse/uncollapse functionality.The test case thoroughly validates the accordion behavior of the override section.
frontend/language/src/nb.json (1)
1448-1464: LGTM! Well-structured translations for Summary2 configuration.The new translations for Summary2 override functionality are clear, consistent, and follow the established naming patterns.
...or/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14460 +/- ##
=======================================
Coverage 95.70% 95.70%
=======================================
Files 1901 1902 +1
Lines 24739 24755 +16
Branches 2833 2833
=======================================
+ Hits 23676 23692 +16
Misses 802 802
Partials 261 261 ☔ View full report in Codecov by Sentry. |
Jondyr
force-pushed
the
08-01-summary2-overrides-design-update
branch
from
54132d3 to
72574c1
Compare
Jondyr
force-pushed
the
20-01-summary2-reorganize-config
branch
from
13b00cd to
429634d
Compare
Jondyr
force-pushed
the
20-01-summary2-reorganize-config
branch
from
429634d to
7373c87
Compare
Jondyr
added
the
text/content
Jondyr
force-pushed
the
08-01-summary2-overrides-design-update
branch
from
ef7c212 to
9937a7a
Compare
linting
Jondyr
force-pushed
the
08-01-summary2-overrides-design-update
branch
from
6f2a2fe to
594a238
Compare
Jondyr
force-pushed
the
20-01-summary2-reorganize-config
branch
from
7373c87 to
aa848ad
Compare
Jondyr
force-pushed
the
20-01-summary2-reorganize-config
branch
from
aa848ad to
ded0d41
Compare
Jondyr
changed the title
Description
Move summary2 configuration to top-level/main configuration
PR Stack
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation