feat(summary2): add new props to overrides #14644
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThis PR introduces updates to the summary override configuration by adding new language keys for summary display options in the JSON file. Type definitions related to summary configurations have been restructured, removing deprecated types and introducing new ones. Several components in the UX editor have been renamed or simplified, with obsolete logic removed and new components introduced to manage display settings more effectively. Test files have been updated to reflect the new naming conventions and type changes. 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx (1)
11-27: LGTM! Component renaming improves clarity.The renaming to
Summary2OverrideCompactSwitchbetter reflects the component's role. The simplified implementation correctly handles the compact view state.Consider adding a default value for
isCompactin the props to avoid the nullish coalescing operator:type CompactViewSwitchProps = { onChange: (updatedOverride: Summary2OverrideConfig) => void; override: Summary2OverrideConfig; + defaultIsCompact?: boolean; }; -export const Summary2OverrideCompactSwitch = ({ onChange, override }: CompactViewSwitchProps) => { +export const Summary2OverrideCompactSwitch = ({ onChange, override, defaultIsCompact = false }: CompactViewSwitchProps) => { // ... - checked={override.isCompact ?? false} + checked={override.isCompact ?? defaultIsCompact}frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx (1)
1-31: LGTM! Test updates maintain coverage.The type updates are correctly implemented, and existing test cases remain valid.
Consider adding test cases for edge cases:
it('should handle undefined displayType', () => { const result = mapSelectedTypeToConfig({ displayType: undefined as unknown as OverrideDisplayType, componentId }); expect(result.displayType).toBeUndefined(); }); it('should handle empty componentId', () => { const result = mapSelectedTypeToConfig({ displayType: stringSelectedType, componentId: '' }); expect(result.componentId).toBe(''); });frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplaySelect.tsx (1)
20-23: Consider extracting options to a constant.The options array could be moved outside the component to prevent recreation on each render.
+const DISPLAY_OPTIONS = ['table', 'full'] as const; + export const Summary2OverrideDisplaySelect = ({ onChange, override, }: Summary2OverrideDisplaySelectProps) => { const { t } = useTranslation(); - const options = [ - { value: 'table', text: t('ux_editor.component_properties.summary.override.display.table') }, - { value: 'full', text: t('ux_editor.component_properties.summary.override.display.full') }, - ]; + const options = DISPLAY_OPTIONS.map((value) => ({ + value, + text: t(`ux_editor.component_properties.summary.override.display.${value}`), + }));frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
123-152: Consider adding error handling for unknown component types.The switch statement should handle unknown component types gracefully.
switch (selectedComponent.type) { case ComponentType.RepeatingGroup: case ComponentType.Subform: return <Summary2OverrideDisplaySelect onChange={onChange} override={override} />; case ComponentType.Checkboxes: case ComponentType.MultipleSelect: return <Summary2OverrideDisplayType onChange={onChange} override={override} />; case ComponentType.Group: return <Summary2OverrideCompactSwitch onChange={onChange} override={override} />; default: + console.warn(`No specific configuration for component type: ${selectedComponent.type}`); return null; }frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
2-2: Remove unused import.The
actimport is declared but never used.-import { act, screen, waitFor } from '@testing-library/react'; +import { screen, waitFor } from '@testing-library/react';🧰 Tools
🪛 GitHub Check: Typechecking and linting
[failure] 2-2:
'act' is declared but its value is never read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/types/ComponentSpecificConfig.ts(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/Summary2OverrideDisplaySelect.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(8 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx(4 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx (1)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts (1)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (3)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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/Summary2OverrideDisplaySelect.tsx (3)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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/CompactViewSwitch.tsx (2)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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 (2)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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/shared/src/types/ComponentSpecificConfig.ts (1)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
🪛 GitHub Check: Typechecking and linting
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx
[failure] 2-2:
'act' is declared but its value is never read.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (14)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts (1)
1-9: LGTM! Type updates are consistent.The replacement of
SummaryCustomTargetTypewithOverrideDisplayTypeis implemented correctly and maintains type safety throughout the file.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (3)
4-6: LGTM! Type import changes look good.The replacement of
SummaryCustomTargetTypewithOverrideDisplayTypealigns with the new type system.
24-30: LGTM! Function signature update is correct.The function correctly uses the new
OverrideDisplayTypetype for the display type parameter.
37-38: LGTM! Event handler update is correct.The event handler correctly casts the value to
OverrideDisplayType.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (3)
61-72: LGTM! Default configuration function is well-structured.The function provides sensible default display configurations for different component types.
74-78: LGTM! Target change handler is well-implemented.The function correctly applies default configurations when changing the target component.
98-102: LGTM! Component-specific configuration is well-organized.The new component centralizes the display configuration logic based on component type.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (3)
185-195: LGTM! Test for component-specific overrides is comprehensive.The test correctly verifies the display type selection for specific components.
197-218: LGTM! Default props test is well-structured.The test effectively verifies that default configurations are applied correctly.
243-261: LGTM! Helper functions are well-organized.The new helper functions improve test readability and maintainability.
frontend/packages/shared/src/types/ComponentSpecificConfig.ts (3)
135-136: LGTM! New display types are well-defined.The separation into
OverrideDisplayTypeandOverrideDisplayimproves type safety.
142-143: LGTM! Summary override config updates are correct.The type correctly uses the new display types for configuration.
146-146: LGTM! Summary target type is well-defined.The reintroduced type clearly specifies valid target types.
frontend/language/src/nb.json (1)
1493-1495: LGTM! The new translations for summary display options are clear and consistent.The added Norwegian translations for display types ("Visningstype", "Fullstendig", "Tabell") are well-formed and follow the established naming conventions.
.../componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplaySelect.tsx
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...r/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx
Fixed
Show fixed
Hide fixed
Jondyr
changed the title
Jondyr
changed the title
Jondyr
force-pushed
the
spr/main/a7e89d34
branch
from
c868379 to
f01c1f9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14644 +/- ##
=======================================
Coverage 95.75% 95.76%
=======================================
Files 1913 1914 +1
Lines 24925 24932 +7
Branches 2848 2848
=======================================
+ Hits 23868 23875 +7
Misses 799 799
Partials 258 258 ☔ View full report in Codecov by Sentry. |
Jondyr
force-pushed
the
spr/main/a7e89d34
branch
from
f01c1f9 to
95b720e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
61-78: Add null check for componentType.The
onChangeTargetfunction should handle the case wherecomponentTypeis undefined.Apply this diff to add a null check:
const onChangeTarget = (value: string) => { const componentType = componentOptions.find((comp) => comp.id === value)?.type; - const defaults = defaultOverrideConfig(componentType); + const defaults = componentType ? defaultOverrideConfig(componentType) : {}; onChange({ componentId: value, ...defaults }); };
123-152: Consider adding error boundaries.The component handles different display types but could benefit from error boundaries to gracefully handle rendering errors.
Consider wrapping the switch statement in an error boundary component to handle potential rendering errors gracefully. This would improve the robustness of the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/types/ComponentSpecificConfig.ts(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/Summary2OverrideDisplaySelect.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(7 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx(4 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts(1 hunks)frontend/packages/ux-editor/src/testing/layoutMock.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplaySelect.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx
- frontend/language/src/nb.json
🧰 Additional context used
🧠 Learnings (3)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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/Summary2Override.test.tsx (1)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
frontend/packages/shared/src/types/ComponentSpecificConfig.ts (1)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (4)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
1-21: LGTM!The imports are well-organized and all dependencies are used in the code.
frontend/packages/ux-editor/src/testing/layoutMock.ts (1)
66-73: LGTM!The
subformComponentMockis well-structured and follows the established pattern of other component mocks.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
188-226: LGTM!The new test cases provide good coverage for component-specific overrides and default props. The test helpers are well-named and reusable.
frontend/packages/shared/src/types/ComponentSpecificConfig.ts (1)
135-144: LGTM!The new type definitions are clear and enhance type safety for override configurations.
Jondyr
force-pushed
the
spr/main/a7e89d34
branch
from
95b720e to
fa93fdd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (3)
48-56: Consider improving test readability with shared setup.Multiple test cases follow a similar pattern of setting up the user event and clicking the override collapsed button. Consider extracting this common setup into a helper function to reduce duplication and improve maintainability.
Example implementation:
+const setupOverrideTest = async (user: UserEvent, props?: Partial<Summary2OverrideProps>) => { + render(props); + await user.click(overrideCollapsedButton(1)); + return user; +}; it('should be able to remove override', async () => { const user = userEvent.setup(); - render({ + await setupOverrideTest(user, { overrides: [{ componentId: '1' }], }); - await user.click(overrideCollapsedButton(1)); await user.click(removeOverrideButton()); expect(defaultProps.onChange).toHaveBeenCalledWith([]); });Also applies to: 151-152, 161-163, 171-186
188-226: Enhance test coverage with additional assertions.The new test cases verify the basic functionality but could be more thorough:
- For component-specific overrides, consider testing invalid display types
- For default props, verify that the component maintains its state after updates
Example implementation for additional test cases:
it('should handle invalid display types gracefully', async () => { const user = userEvent.setup(); render({ overrides: [{ componentId: container2IdMock }] }); await user.click(overrideCollapsedButton(1)); await user.selectOptions(overrideDisplaySelector(), overrideDisplaySelectType('invalid_type')); expect(defaultProps.onChange).not.toHaveBeenCalled(); }); it('should maintain state after multiple updates', async () => { const user = userEvent.setup(); const initialState = { componentId: container2IdMock, display: 'table' }; render({ overrides: [initialState] }); await user.click(overrideCollapsedButton(1)); await user.selectOptions(overrideDisplaySelector(), overrideDisplaySelectType('full')); await user.selectOptions(overrideDisplaySelector(), overrideDisplaySelectType('table')); expect(defaultProps.onChange).toHaveBeenLastCalledWith( expect.arrayContaining([expect.objectContaining(initialState)]) ); });
251-269: Add TypeScript types to improve maintainability.Consider adding TypeScript types for the display and display type values to improve code maintainability and catch potential errors early.
Example implementation:
+type DisplayType = 'table' | 'full'; +type DisplaySelectType = 'string' | 'list'; -const overrideDisplaySelectType = (type: string) => +const overrideDisplaySelectType = (type: DisplayType) => screen.getByRole('option', { name: textMock(`ux_editor.component_properties.summary.override.display.${type}`), }); -const overrideDisplayType = (type: string) => +const overrideDisplayType = (type: DisplaySelectType) => screen.getByRole('option', { name: textMock(`ux_editor.component_properties.summary.override.display_type.${type}`), });frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
129-152: Add error handling for invalid component types.While the component correctly handles known component types, it silently returns null for unknown types. Consider adding error handling or logging to help identify unexpected component types during development.
Apply this diff to add error handling:
if (!selectedComponent) { return null; } + const { type } = selectedComponent; + if (!Object.values(ComponentType).includes(type)) { + console.warn(`Unexpected component type: ${type}`); + return null; + } - switch (selectedComponent.type) { + switch (type) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/types/ComponentSpecificConfig.ts(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/Summary2OverrideDisplaySelect.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(7 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx(4 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts(1 hunks)frontend/packages/ux-editor/src/testing/layoutMock.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts
- frontend/packages/ux-editor/src/testing/layoutMock.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplaySelect.tsx
- frontend/language/src/nb.json
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx
- frontend/packages/shared/src/types/ComponentSpecificConfig.ts
🧰 Additional context used
🧠 Learnings (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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/Summary2Override.test.tsx (1)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
- GitHub Check: Typechecking and linting
🔇 Additional comments (3)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
15-17: LGTM! Mock data setup is well-structured.The new mock data is properly imported and integrated into the existing test setup.
Also applies to: 24-24, 30-30
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
1-21: LGTM! Well-organized imports.The imports are logically grouped and follow a clear organization pattern.
74-78: LGTM! Well-implemented target change handler.The function correctly:
- Retrieves the component type
- Applies appropriate default configuration
- Updates the override with new values
...r/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx
Outdated
Show resolved
Hide resolved
Jondyr
force-pushed
the
spr/main/a7e89d34
branch
2 times, most recently
from
9d9063b to
6209ea9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
74-83: Maintain consistency in userEvent usage.This test case uses the old
userEventapproach while other test cases use the newerconst user = userEvent.setup()pattern.Apply this change for consistency:
- await userEvent.click(overrideCollapsedButton(1)); + const user = userEvent.setup(); + await user.click(overrideCollapsedButton(1));frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (1)
37-37: Consider improving default value handling.The current default fallback to 'list' in the value prop could be made more explicit and type-safe.
Consider this improvement:
- value={displayType || 'list'} + value={displayType ?? 'list' as OverrideDisplayType}This change:
- Uses the nullish coalescing operator for more precise default handling
- Ensures type safety for the default value
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
74-78: Add error handling for undefined componentType.The
onChangeTargetfunction should handle cases wherecomponentTypeis undefined to prevent potential runtime issues.Consider this improvement:
const onChangeTarget = (value: string) => { const componentType = componentOptions.find((comp) => comp.id === value)?.type; - const defaults = defaultOverrideConfig(componentType); + const defaults = componentType ? defaultOverrideConfig(componentType) : {}; onChange({ componentId: value, ...defaults }); };
123-152: Consider extracting component type groups into constants.The component type groupings in the switch statement could be made more maintainable by extracting them into named constants.
Consider this improvement:
+const DISPLAY_SELECT_TYPES = [ComponentType.RepeatingGroup, ComponentType.Subform] as const; +const DISPLAY_TYPE_TYPES = [ComponentType.Checkboxes, ComponentType.MultipleSelect] as const; +const COMPACT_SWITCH_TYPES = [ComponentType.Group] as const; const Summary2OverrideComponentSpecificConfig = ({ onChange, override, componentOptions, }: Summary2OverrideComponentSpecificConfigProps) => { const selectedComponent = componentOptions?.find((comp) => comp.id === override.componentId); if (!selectedComponent) { return null; } switch (selectedComponent.type) { - case ComponentType.RepeatingGroup: - case ComponentType.Subform: + case DISPLAY_SELECT_TYPES.includes(selectedComponent.type): return <Summary2OverrideDisplaySelect onChange={onChange} override={override} />; - case ComponentType.Checkboxes: - case ComponentType.MultipleSelect: + case DISPLAY_TYPE_TYPES.includes(selectedComponent.type): return <Summary2OverrideDisplayType onChange={onChange} override={override} />; - case ComponentType.Group: + case COMPACT_SWITCH_TYPES.includes(selectedComponent.type): return <Summary2OverrideCompactSwitch onChange={onChange} override={override} />; default: return null; } };This change would:
- Make it easier to maintain groups of related component types
- Make the switch statement more readable
- Reduce the likelihood of errors when adding new component types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/types/ComponentSpecificConfig.ts(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/Summary2OverrideDisplaySelect.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(7 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx(4 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts(1 hunks)frontend/packages/ux-editor/src/testing/layoutMock.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/packages/ux-editor/src/testing/layoutMock.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplaySelect.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts
- frontend/packages/shared/src/types/ComponentSpecificConfig.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx
- frontend/language/src/nb.json
🧰 Additional context used
🧠 Learnings (3)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (3)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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/Summary2OverrideEntry.tsx (2)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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/Summary2Override.test.tsx (1)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
🔇 Additional comments (8)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (3)
15-17: LGTM!The new imports and type definitions are well-structured and properly typed.
Also applies to: 21-24
192-230: LGTM!The new test cases are well-structured and provide good coverage for component-specific overrides and default props. The use of parameterized testing with
it.eachis particularly effective.
255-273: LGTM!The helper functions are well-organized, properly typed, and follow consistent naming patterns. The use of specific role selectors enhances test reliability.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (3)
5-5: LGTM! Type import change aligns with the new display property.The replacement of
SummaryCustomTargetTypewithOverrideDisplayTypemaintains proper type safety while supporting the new display property functionality.
24-24: LGTM! Function signature updated with new type.The function maintains its core functionality while adopting the new
OverrideDisplayTypefor improved type safety.
38-38: LGTM! Event handler updated with correct type casting.The onChange handler correctly uses the new
OverrideDisplayTypefor type casting, maintaining type safety throughout the component.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
1-21: LGTM!The import statements are well-organized, and the type definitions are clear and well-structured.
61-72: LGTM!The
defaultOverrideConfigfunction correctly implements the default display values as specified in the PR objectives:
fullfor RepeatingGrouptablefor Subform
Jondyr
force-pushed
the
spr/main/a7e89d34
branch
from
6209ea9 to
0e036ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
123-152: Consider adding error handling for unknown component types.The
Summary2OverrideComponentSpecificConfigcomponent handles known component types well, but the default case silently returns null.Consider adding a warning or logging for unknown component types:
- default: - return null; + default: + console.warn(`Unsupported component type: ${selectedComponent.type}`); + return null;frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
255-273: Consider consolidating similar selector functions.The
overrideDisplaySelector,overrideDisplaySelectType, andoverrideDisplayTypefunctions share similar patterns.Consider creating a generic selector function:
const createOptionSelector = (role: string, nameKey: string) => (type?: string) => screen.getByRole(role, { name: type ? textMock(`ux_editor.component_properties.summary.override.${nameKey}.${type}`) : textMock(`ux_editor.component_properties.summary.override.${nameKey}`), }); const overrideDisplaySelector = createOptionSelector('combobox', 'display'); const overrideDisplaySelectType = (type: OverrideDisplay) => createOptionSelector('option', 'display')(type); const overrideDisplayType = (type: OverrideDisplayType) => createOptionSelector('option', 'display_type')(type);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/types/ComponentSpecificConfig.ts(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/Summary2OverrideDisplaySelect.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(7 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx(4 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts(1 hunks)frontend/packages/ux-editor/src/testing/layoutMock.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx
- frontend/packages/ux-editor/src/testing/layoutMock.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx
- frontend/packages/shared/src/types/ComponentSpecificConfig.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplaySelect.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx
- frontend/language/src/nb.json
🧰 Additional context used
🧠 Learnings (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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/Summary2Override.test.tsx (1)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
🔇 Additional comments (6)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (3)
9-21: LGTM!The imports are well-organized and the types are properly imported from their respective modules.
61-72: LGTM!The
defaultOverrideConfigfunction correctly implements the default display values as specified in the PR objectives:
fullfor RepeatingGrouptablefor Subform
74-78: LGTM!The
onChangeTargetfunction effectively combines component selection with default configuration initialization.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (3)
21-24: LGTM!The type imports are properly structured and named according to the new type system.
192-202: LGTM!The test case for component-specific overrides effectively verifies the display selection functionality.
204-230: LGTM!The parameterized test case thoroughly verifies default props for different component types.
lassopicasso
approved these changes
Feb 13, 2025
...r/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx
Show resolved
Hide resolved
...r/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx
Show resolved
Hide resolved
Adds a new property to overrides for RepeatingGroup and Subform `display`. This property has a default value dependent on which component is targeted. RepeatingGroup is `full` and Subform is `table` (see layout.schema.v1.json) commit-id:a7e89d34
Jondyr
force-pushed
the
spr/main/a7e89d34
branch
from
0e036ef to
ff22349
Compare
4 tasks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
74-78: Consider handling undefined componentType explicitly.While the current implementation works, it would be more robust to handle the case where componentType is undefined explicitly.
const onChangeTarget = (value: string) => { const componentType = componentOptions.find((comp) => comp.id === value)?.type; + if (!componentType) { + onChange({ componentId: value }); + return; + } const defaults = defaultOverrideConfig(componentType); onChange({ componentId: value, ...defaults }); };
123-152: Consider extracting component type mapping to a constant.The component effectively manages display configurations, but the switch statement could be more maintainable if the component type mapping was extracted to a constant.
+const COMPONENT_DISPLAY_MAP = { + [ComponentType.RepeatingGroup]: Summary2OverrideDisplaySelect, + [ComponentType.Subform]: Summary2OverrideDisplaySelect, + [ComponentType.Checkboxes]: Summary2OverrideDisplayType, + [ComponentType.MultipleSelect]: Summary2OverrideDisplayType, + [ComponentType.Group]: Summary2OverrideCompactSwitch, +} as const; + const Summary2OverrideComponentSpecificConfig = ({ onChange, override, componentOptions, }: Summary2OverrideComponentSpecificConfigProps) => { const selectedComponent = componentOptions?.find((comp) => comp.id === override.componentId); if (!selectedComponent) { return null; } - switch (selectedComponent.type) { - case ComponentType.RepeatingGroup: - case ComponentType.Subform: - return <Summary2OverrideDisplaySelect onChange={onChange} override={override} />; - case ComponentType.Checkboxes: - case ComponentType.MultipleSelect: - return <Summary2OverrideDisplayType onChange={onChange} override={override} />; - case ComponentType.Group: - return <Summary2OverrideCompactSwitch onChange={onChange} override={override} />; - default: - return null; - } + const DisplayComponent = COMPONENT_DISPLAY_MAP[selectedComponent.type]; + return DisplayComponent ? <DisplayComponent onChange={onChange} override={override} /> : null; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/language/src/nb.json(1 hunks)frontend/packages/shared/src/types/ComponentSpecificConfig.ts(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/Summary2OverrideDisplaySelect.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(7 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx(4 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts(1 hunks)frontend/packages/ux-editor/src/testing/layoutMock.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/packages/ux-editor/src/testing/layoutMock.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/utils.test.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplaySelect.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx
- frontend/language/src/nb.json
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx
- frontend/packages/shared/src/types/ComponentSpecificConfig.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
Learnt from: lassopicasso
PR: Altinn/altinn-studio#14575
File: frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx:47-58
Timestamp: 2025-02-07T09:50:33.290Z
Learning: Avoid suggesting StudioCombobox component in future PRs for the Altinn Studio repository. A new replacement component will be introduced later.
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 (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
1-21: LGTM! Import statements are well-organized.The imports are logically grouped and all imports are necessary for the added functionality.
61-72: LGTM! Default override configurations are correctly implemented.The function properly sets default display values according to the PR objectives:
- 'full' for RepeatingGroup
- 'table' for Subform
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds a new property to overrides for RepeatingGroup and Subform
display.This property has a default value dependent on which component is
targeted. RepeatingGroup is
fulland Subform istable(seelayout.schema.v1.json)
Stack:
Related Issue(s)
SubformandRepeatingGroupinSummary2overrides #14537Verification
Documentation
Summary by CodeRabbit
New Features
Refactor