feat: update summary2 override configuration design #14379
Conversation
github-actions
bot
added
area/ui-editor
Jondyr
changed the title
Jondyr
force-pushed
the
08-01-summary2-overrides-design-update
branch
3 times, most recently
from
55474ad to
cde2806
Compare
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the Summary2 component configuration system in the frontend. The changes focus on improving the flexibility of component summaries by adding new configuration options such as Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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 (
|
Jondyr
force-pushed
the
08-01-summary2-overrides-design-update
branch
from
09934ee to
c00d7a5
Compare
![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
...r/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx
Fixed
Show fixed
Hide fixed
...r/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx
Fixed
Show fixed
Hide fixed
...r/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx
Fixed
Show fixed
Hide fixed
Jondyr
force-pushed
the
08-01-summary2-overrides-design-update
branch
from
ff061f7 to
7a69f47
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.module.css (2)
1-9: Consider adding gap property fallback for broader browser support.While the styling looks good and follows the design system, consider adding a fallback for the gap property to support older browsers.
.card { margin-top: var(--fds-spacing-4); margin-bottom: var(--fds-spacing-4); + margin-right: calc(var(--fds-spacing-4) * -1); /* Fallback */ gap: var(--fds-spacing-4); + display: flex; /* Required for gap in older browsers */ + flex-direction: column; }
26-29: Consider adding flex-wrap for better responsive behavior.The button group styling looks good with consistent spacing. Consider adding
flex-wrap: wrapfor better handling of narrow viewports..buttongroup { display: flex; gap: var(--fds-spacing-4); + flex-wrap: wrap; }frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
102-110: Use Descriptive Button TitlesThe
StudioButtonfor saving changes uses thetitleprop set tot('general.save'), which might not be sufficient for accessibility. Ensure that the button has an accessible label and consider adding anaria-labelif necessary.<StudioButton icon={<CheckmarkIcon />} type='submit' title={t('general.save')} + aria-label={t('general.save_changes')} variant='secondary' color='success' onClick={() => setOpen(false)} />frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx (1)
20-21: Ensure Switch Checked State ReflectshiddenProperty CorrectlyThe
checkedprop is set as!override.hidden, which might be counterintuitive. To improve readability, consider aligning thecheckedstate directly with theoverride.hiddenproperty.- checked={!override.hidden} + checked={override.hidden}If making this change, remember to adjust the
onChangelogic accordingly.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx (1)
25-26: AligncheckedState with Property LogicThe
checkedprop is currently set to!override.hideEmptyFields, which may be confusing. For better readability, consider aligning thecheckedstate directly with theoverride.hideEmptyFieldsproperty.- checked={!override.hideEmptyFields} + checked={override.hideEmptyFields}If you make this change, ensure to adjust the
onChangehandler logic accordingly.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx (1)
24-31: Consider removing empty icon prop.The icon prop is set to an empty string which seems unnecessary. Consider removing it if no icon is needed.
inputProps={{ - icon: '', label: t('ux_editor.component_properties.summary.override.empty_field_text'), size: 'sm', value: override.emptyFieldText ?? '', onChange: (event: React.ChangeEvent<HTMLInputElement>) => onChange('emptyFieldText', event.target.value), }}frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (1)
29-35: Consider optimizing component type check.The component type check could be:
- Extracted to a utility function for reusability
- Memoized to prevent unnecessary recalculations
+ const isCheckboxOrMultipleselect = React.useMemo( + () => + override.componentId.includes(ComponentType.MultipleSelect) || + override.componentId.includes(ComponentType.Checkboxes), + [override.componentId] + ); - const checkboxOrMultipleselect = - override.componentId.includes(ComponentType.MultipleSelect) || - override.componentId.includes(ComponentType.Checkboxes); - if (!checkboxOrMultipleselect) { + if (!isCheckboxOrMultipleselect) {frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx (1)
33-37: Prevent potential memory leaks in state updates.The state update doesn't properly handle cleanup of old state entries.
onHeaderClick={() => setAccordionOpen((prev) => { - return { ...prev, summary2overrides: !prev['summary2overrides'] }; + const { summary2overrides, ...rest } = prev; + return { ...rest, summary2overrides: !summary2overrides }; }) }frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
44-67: Consider adding error boundaries for robust error handling.While the rendering logic is sound, consider wrapping the component tree in error boundaries to gracefully handle potential runtime errors.
+import { ErrorBoundary } from '@studio/components'; export const Summary2Override = ({ overrides, onChange }: Summary2OverrideProps) => { // ... existing code ... return ( + <ErrorBoundary fallback={<div>Error loading overrides</div>}> <> {/* existing JSX */} </> + </ErrorBoundary> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
frontend/language/src/nb.json(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/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)
💤 Files with no reviewable changes (1)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/index.ts
⏰ 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 (13)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.module.css (3)
11-13: Verify the padding-left value.The use of
--fds-spacing-0for padding seems unusual. Did you mean to use a different spacing value? If this is intentional, consider adding a comment explaining why zero padding is needed.
15-18: LGTM!The content styling follows the design system spacing conventions consistently.
20-24: LGTM!The divider styling is well-implemented using semantic color variables, which ensures proper theme compatibility.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/hook/useCustomConfigType.ts (1)
11-22: LGTM! Verify translation keys exist.The reorganization of translation keys follows a more structured and maintainable naming convention. The changes align well with the Summary2 override configuration design update.
Let's verify the new translation keys exist:
✅ Verification successful
Translation keys verified successfully
All required translation keys are properly defined in the Norwegian language file (
frontend/language/src/nb.json) and are consistently used across the Summary2 override components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new translation keys exist in the translation files # Find and check all translation files (common locations for React apps) rg -l "ux_editor\.component_properties\.summary\.override\.display_type\." \ --type json \ --type yaml \ --type typescript \ -g "*.{json,yaml,yml,ts}"Length of output: 179
Script:
#!/bin/bash # First, find translation-related files echo "=== Finding translation files ===" fd -e json -e ts "language|translation|i18n|locale" --type f echo -e "\n=== Searching for translation keys ===" # Search with proper file type flags and more flexible pattern rg "ux_editor\.component_properties\.summary\.override\.display_type" \ --type ts \ --type json \ -g '!*test*' \ -g '!*spec*' \ -A 2 -B 2Length of output: 4970
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
87-92: Ensure Consistent Handling of Hidden OverridesWhen
override.hiddenistrue, an alert is displayed. Verify that this behavior aligns with the intended user experience and that the alert message provides clear guidance.If this is expected, no action is needed. Otherwise, consider adjusting the logic or the alert content for clarity.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx (2)
6-9: LGTM! Props interface is well-defined.The TypeScript interface for props is clear and properly typed.
1-41: Add unit tests for the new component.Since this is a new component, ensure it has proper test coverage, especially for:
- Rendering with hideEmptyFields true/false
- onChange handler functionality
- Empty field text display
Would you like me to help generate the unit test suite for this component?
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx (1)
53-53: LGTM! Clean integration of ComponentMainConfig.The ComponentMainConfig is properly integrated with correct prop passing.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx (2)
11-14: LGTM! Props interface is well-defined.The interface clearly specifies the required props with appropriate types.
16-43: LGTM! Component implementation is clean and efficient.The component:
- Correctly validates component type before rendering
- Properly handles state management
- Uses appropriate null checks
- Implements efficient data transformations
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
Line range hint
15-41: LGTM! State management is well-implemented.The component:
- Properly manages override visibility state
- Correctly handles index adjustments on deletion
- Maintains array immutability in state updates
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (1)
Line range hint
1-347: LGTM! Test coverage is comprehensive.The test suite:
- Covers all major component functionality
- Properly tests user interactions
- Handles async operations correctly
- Uses appropriate test utilities and mocks
frontend/language/src/nb.json (1)
1447-1462: LGTM! Translations are complete and consistent.The new translations:
- Follow the established naming convention
- Provide clear Norwegian translations for all new functionality
- Maintain consistency with existing translations
...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
...ponents/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx
Outdated
Show resolved
Hide resolved
...s/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14379 +/- ##
=======================================
Coverage 95.69% 95.69%
=======================================
Files 1888 1893 +5
Lines 24556 24623 +67
Branches 2818 2821 +3
=======================================
+ Hits 23498 23563 +65
- Misses 799 800 +1
- Partials 259 260 +1 ☔ View full report in Codecov by Sentry. |
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)
101-109: Consider adding a success message when saving changesWhile the save button implementation is correct, consider adding a success message or toast notification when changes are saved to provide better user feedback.
Example implementation:
<StudioButton icon={<CheckmarkIcon />} type='submit' title={t('general.save')} variant='secondary' color='success' - onClick={() => setOpen(false)} + onClick={() => { + setOpen(false); + // Add success notification here + showNotification({ + message: t('ux_editor.component_properties.summary.override.save_success'), + type: 'success' + }); + }} />frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx (1)
107-113: Simplify the component tag.The empty closing tag can be self-closed for better readability.
- ></Summary2ComponentReferenceSelector> + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/language/src/nb.json(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.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/Summary2OverrideEntry.tsx(2 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.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2ComponentReferenceSelector.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx
- frontend/language/src/nb.json
🧰 Additional context used
📓 Learnings (1)
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 (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (5)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (3)
17-18: Fix typo in component name 'Summmary2ComponentReferenceSelector'The import statement contains a typo in the component name.
Apply this diff to fix the typo:
- import { Summmary2ComponentReferenceSelector } from '../Summary2ComponentReferenceSelector'; + import { Summary2ComponentReferenceSelector } from '../Summary2ComponentReferenceSelector';
Line range hint
24-41: Well-structured component with proper TypeScript typingThe component structure follows React best practices with proper TypeScript typing and clear prop definitions.
62-75: Clean implementation of conditional rendering and state managementThe component correctly implements conditional rendering based on the 'open' state, and the StudioProperty.Button usage follows the established pattern for handling undefined values.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx (2)
Line range hint
1-124: Well-structured implementation!The component is well-implemented with:
- Proper TypeScript typing
- Consistent error handling
- Clean integration with the design system
- Clear separation of concerns
Line range hint
98-106: Verify the workaround and track its removal.The key prop is used as a workaround for a design system issue. Let's verify the status of this issue to ensure we can remove the workaround when it's fixed.
✅ Verification successful
Workaround is still required
The key prop workaround is still necessary as the linked issue (digdir/designsystemet#2264) remains open. The TODO comment is well-documented and correctly tracks the dependency for future removal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the status of the design system issue # that necessitates the key prop workaround gh issue view 2264 --repo digdir/designsystemetLength of output: 1419
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx (2)
1-9: Align file name, type name, and component name for consistency.The file name
ForceShowSwitch.tsxand type nameForceShowSwitchPropsdon't match the exported component nameOverrideShowComponentSwitch. Consider renaming for consistency:
- Rename file to
OverrideShowComponentSwitch.tsx- Rename type to
OverrideShowComponentSwitchProps
21-21: Remove unnecessary value prop.The
valueprop set to 'hidden' doesn't serve any purpose in the current implementation since it's not used in the onChange handler.- value={'hidden'}frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.test.tsx (3)
21-49: Test coverage could be improvedWhile the current tests cover basic functionality, consider adding the following test cases:
- Error handling scenarios
- Multiple overrides behavior
- Component state after override deletion
Example test case:
it('should handle multiple overrides correctly', async () => { const user = userEvent.setup(); const summary2ComponentWithMultipleOverrides = { ...summary2Component, overrides: [{ componentId: '0' }, { componentId: '1' }], }; render(summary2ComponentWithMultipleOverrides); await user.click(summary2AccordionButton()); expect(summary2CollapsedButton(1)).toBeInTheDocument(); expect(summary2CollapsedButton(2)).toBeInTheDocument(); });
52-59: Improve regex pattern for button nameThe regex pattern for
summary2CollapsedButtonmight be too permissive. The current pattern.*:${n}}could match unintended strings.- name: new RegExp(`ux_editor.component_properties.summary.overrides.nth.*:${n}}`), + name: new RegExp(`ux_editor\\.component_properties\\.summary\\.overrides\\.nth[^:]*:${n}}`),
61-77: Consider extracting test setup logicThe render function contains complex setup logic that could be reused across different test files.
Consider moving this to a shared test utility:
// testUtils.ts export const renderWithLayoutContext = ( component: React.ReactElement, layoutSetName = layoutSet1NameMock, layoutName = layout1NameMock, ) => { const queryClient = createQueryClientMock(); queryClient.setQueryData([QueryKey.FormLayouts, org, app, layoutSetName], { [layoutName]: layoutMock, }); return renderWithProviders(component, { queryClient, appContextProps: { selectedFormLayoutSetName: layoutSetName, selectedFormLayoutName: layoutName, }, }); };frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (3)
57-70: Add aria-label for better accessibilityThe button should have an aria-label to improve screen reader experience.
<StudioProperty.Button className={classes.property} property={t('ux_editor.component_properties.summary.overrides.nth', { n: index })} value={componentNameType && `${componentNameType} (ID:${override.componentId})`} icon={false} onClick={() => setOpen(true)} + aria-label={t('ux_editor.component_properties.summary.overrides.edit_override', { + component: componentNameType || override.componentId + })} />
82-95: Consider extracting conditional rendering logicThe conditional rendering of override settings could be extracted into a separate component for better maintainability.
const OverrideSettings = ({ override, onChange }) => { if (override.hidden) { return ( <StudioAlert> {t('ux_editor.component_properties.summary.override.hide_empty_fields.info_message')} </StudioAlert> ); } return ( <> <StudioDivider className={classes.divider} /> <CompactViewSwitch onChange={onChange} override={override} /> <ShowEmptyFieldSwitch onChange={onChange} override={override} /> <Summary2OverrideDisplayType onChange={onChange} override={override} /> <EmptyTextField onChange={onChange} override={override} /> </> ); };
96-106: Add confirmation for delete actionConsider adding a confirmation dialog before deleting an override to prevent accidental deletions.
- <StudioDeleteButton onDelete={onDelete}></StudioDeleteButton> + <StudioDeleteButton + onDelete={() => { + if (window.confirm(t('ux_editor.component_properties.summary.override.delete_confirmation'))) { + onDelete(); + } + }} + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.test.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/Summary2OverrideEntry.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx
🧰 Additional context used
📓 Learnings (2)
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/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 (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
🔇 Additional comments (1)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx (1)
17-19: LGTM! The onChange logic is correctly implemented.The inversion logic for the
hiddenproperty is properly handled, ensuring the switch state correctly reflects the component's visibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx (5)
375-378: Consider using a more robust regex pattern.The current regex pattern uses string interpolation which could make it fragile if the text contains special regex characters. Consider escaping special characters in the text mock before using it in the regex pattern.
- name: new RegExp(`ux_editor.component_properties.summary.overrides.nth.*:${n}}`), + name: new RegExp(escapeRegExp(`ux_editor.component_properties.summary.overrides.nth.*:${n}}`)),Add this helper function:
function escapeRegExp(string: string) { return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); }
Line range hint
94-104: Enhance test readability with better error messages.The assertion error message is missing, which could make test failures harder to debug. Consider adding a descriptive error message to the assertion.
await waitFor(() => expect(defaultProps.onChange).toHaveBeenCalledWith( - expect.arrayContaining([{ componentId: '1', hidden: true }]), + expect.arrayContaining([{ componentId: '1', hidden: true }]), + 'Component visibility state should be updated correctly' ), );
110-123: Consider extracting common test setup.Multiple test cases share similar setup code. Consider extracting the common setup into a helper function to reduce duplication and improve maintainability.
const setupOverrideTest = async (initialProps: Partial<Override>, user = userEvent.setup()) => { render({ overrides: [{ componentId: '1', ...initialProps }] }); await userEvent.click(overrideCollapsedButton(1)); return user; };Usage:
- const user = userEvent.setup(); - render({ overrides: [{ componentId: '1', hidden: false }] }); - await userEvent.click(overrideCollapsedButton(1)); + const user = await setupOverrideTest({ hidden: false });
343-351: Consider adding more test scenarios for info message.The test only verifies the presence of the info message. Consider adding test cases for:
- Info message not showing when hideEmptyFields is false
- Info message content accuracy
it('should not show info message when hideEmptyFields is false', async () => { render({ overrides: [{ componentId: '1', hideEmptyFields: false }] }); await userEvent.click(overrideCollapsedButton(1)); expect( screen.queryByText( textMock('ux_editor.component_properties.summary.override.hide_empty_fields.info_message') ) ).not.toBeInTheDocument(); });
353-367: Simplify collapse/uncollapse test assertions.The test has duplicate assertions that check for the same condition. Consider simplifying the test to make it more focused and maintainable.
it('should collapse and uncollapse override', async () => { render({ overrides: [{ componentId: '1' }] }); + const buttonQuery = () => + screen.queryByRole('button', { + name: textMock('ux_editor.component_properties.summary.overrides.nth.*:1}'), + }); + await userEvent.click(overrideCollapsedButton(1)); - expect( - screen.queryByRole('button', { - name: textMock('ux_editor.component_properties.summary.overrides.nth.*:1}'), - }), - ).not.toBeInTheDocument(); + expect(buttonQuery()).not.toBeInTheDocument(); + await userEvent.click(overrideCloseButton()); - expect( - screen.queryByRole('button', { - name: textMock('ux_editor.component_properties.summary.overrides.nth.*:1}'), - }), - ).not.toBeInTheDocument(); + expect(buttonQuery()).not.toBeInTheDocument(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx(16 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
Jondyr
force-pushed
the
08-01-summary2-overrides-design-update
branch
from
8198f2d to
a961e3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
Line range hint
24-30: Replaceanytype with specific type.The
onChangeOverridefunction usesanytype which reduces type safety.Apply this fix:
const onChangeOverride = - (index: number): ((override: any) => void) => - (override: any) => { + (index: number): ((override: Summary2OverrideConfig) => void) => + (override: Summary2OverrideConfig) => { const updatedOverrides = [...overrides]; updatedOverrides[index] = override; onChange(updatedOverrides); };
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
73-95: Consider extracting the conditional content into separate components.The nested conditional rendering makes the code harder to maintain. Consider extracting the content inside the
override.hiddencondition into separate components to improve readability and maintainability.+ const HiddenContent = () => ( + <StudioAlert> + {t('ux_editor.component_properties.summary.override.hide_empty_fields.info_message')} + </StudioAlert> + ); + const VisibleContent = ({ override, onChange }) => ( + <> + <StudioDivider className={classes.divider} /> + <CompactViewSwitch onChange={onChange} override={override} /> + <ShowEmptyFieldSwitch onChange={onChange} override={override} /> + <Summary2OverrideDisplayType onChange={onChange} override={override} /> + <EmptyTextField onChange={onChange} override={override} /> + </> + );
96-106: Consider adding explicit spacing in the button group.The button group might benefit from explicit spacing between buttons for consistent layout across different themes and screen sizes.
- <div className={classes.buttongroup}> + <div className={`${classes.buttongroup} ${classes.spacing}`}>Add to CSS module:
.spacing { gap: var(--studio-space-3); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
frontend/language/src/nb.json(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/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.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/index.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/hook/useCustomConfigType.ts
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2ComponentReferenceSelector.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.module.css
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.test.tsx
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx
- frontend/language/src/nb.json
🧰 Additional context used
📓 Learnings (2)
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/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 (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (6)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (3)
1-23: LGTM! Well-structured imports and type definitions.The imports are logically organized, and the type definition for
Summary2OverrideEntryPropsis clear and properly documented with the new props.Also applies to: 24-32
Line range hint
33-56: LGTM! Clean component setup with proper hook usage.The component initialization and data processing logic is well-structured. The hooks are used appropriately, and the component options are efficiently mapped from the form layout data.
57-70: Previous discussion about undefined handling is still valid.The implementation correctly follows the established pattern for StudioProperty.Button behavior with undefined values.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (3)
Line range hint
1-13: Clean imports and well-structured type definitions!The imports are organized logically, and the type definitions clearly specify the component's interface.
17-22: Add validation for duplicate componentIds.The
addOverridefunction doesn't validate if the new override's componentId is unique.Consider adding validation to prevent duplicate componentIds. Would you like me to provide an implementation for this validation?
44-67: Well-structured render logic with proper state management!The UI implementation effectively manages the visibility state of override entries and provides clear user interaction patterns.
...editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx
Outdated
Show resolved
Hide resolved
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx (1)
22-41: Consider these refinements for better maintainability.
- Remove the empty icon prop since it's not being used.
- Move inline styles to a CSS module for better maintainability.
- icon: '', + // Remove empty icon prop - style: { padding: '0' }, + className: classes.textfield // Add to CSS module: + .textfield { + padding: 0; + }frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
51-55: Simplify the setOpen callback.The conditional expression can be written more concisely.
- 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/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
96-106: Improve button group implementation.Consider these refinements:
- Rename the CSS class to be more specific (e.g.,
overrideActionButtons).- Remove
type='submit'from the save button as it's not within a form and uses onClick handler.- <div className={classes.buttongroup}> + <div className={classes.overrideActionButtons}> - type='submit' + type='button'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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/ShowEmptyFieldsSwitch.tsx(1 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.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx
🧰 Additional context used
📓 Learnings (3)
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/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/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 (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (5)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx (2)
1-10: Clean and well-structured type definitions!The imports are organized properly, and the props interface is well-defined with clear types.
11-20: Conditional rendering logic aligns with design intent!The condition
override.hideEmptyFields || !override.forceShowcorrectly implements the intended behavior as per the retrieved learning.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
Line range hint
15-41: State management implementation looks good!The state updates are now handled immutably, and the index management in
onDeleteOverridecorrectly maintains the array indices after deletion.frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
Line range hint
1-41: Well-structured imports and props!The imports are properly organized, and the props interface is well-defined with the new properties.
57-70: Correct implementation of undefined handling!The code correctly implements the expected behavior of passing
falsewhencomponentNameTypeis undefined, as per the retrieved learning.
✅ Actions performedComments resolved. |
1 similar comment
✅ Actions performedComments resolved. |
Jondyr
changed the base branch from
main
to
13-01-new-summary2-translation-keys-and-reorganize
...src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.module.css
Outdated
Show resolved
Hide resolved
Jondyr
force-pushed
the
08-01-summary2-overrides-design-update
branch
from
54132d3 to
72574c1
Compare
…cificContent/Summary2/Override/Summary2Override.module.css
Jondyr
force-pushed
the
08-01-summary2-overrides-design-update
branch
from
6f2a2fe to
594a238
Compare
|
@Annikenkbrathen thanks for the descriptive feedback! summary2-override-ux-feedback.mp4 |
Nice!
I did also notice that we miss on config switch "Bruk kompakt visning" today (did not see that yesterday). Is it any reason that it's not included? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx (1)
1-26: LGTM! Clean implementation with proper type safety.The component follows React best practices and correctly implements the switch behavior for controlling the
hiddenproperty in Summary2 overrides.Consider adding prop validation.
For better runtime safety, consider adding prop-types or runtime validation to ensure required props are provided:
+ if (!override) { + throw new Error('override prop is required for OverrideShowComponentSwitch'); + }frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
73-95: Consider improving the conditional rendering structure.The nested conditional rendering with multiple components could be simplified for better maintainability.
Consider extracting the content inside the else block into a separate component:
- {override.hidden ? ( - <StudioAlert> - {t('ux_editor.component_properties.summary.override.hide_empty_fields.info_message')} - </StudioAlert> - ) : ( - <> - <StudioDivider className={classes.divider} /> - <CompactViewSwitch onChange={onChange} override={override} /> - <Summary2OverrideDisplayType onChange={onChange} override={override} /> - <ShowEmptyFieldSwitch onChange={onChange} override={override} /> - <EmptyTextField onChange={onChange} override={override} /> - </> - )} + {override.hidden ? ( + <StudioAlert> + {t('ux_editor.component_properties.summary.override.hide_empty_fields.info_message')} + </StudioAlert> + ) : ( + <OverrideContent + onChange={onChange} + override={override} + /> + )}
96-106: Consider adding loading state to the save button.The save button might benefit from a loading state to prevent double submissions.
Consider adding a loading state:
<StudioButton icon={<CheckmarkIcon />} type='submit' title={t('general.save')} variant='secondary' color='success' + loading={isSaving} onClick={() => { + setIsSaving(true); setOpen(false); + setIsSaving(false); }} />frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
37-39: Simplify the openOverrides state update logic.The nested logic for filtering and mapping indices could be made more readable.
Consider this alternative:
- setOpenOverrides((prev) => { - return prev.filter((i) => i !== index).map((i) => (i > index ? i - 1 : i)); - }); + setOpenOverrides((prev) => prev + .filter((i) => i !== index) + .map((i) => (i > index ? i - 1 : i)) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
frontend/language/src/nb.json(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/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.test.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(17 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/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.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/index.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2ComponentReferenceSelector.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/Summary2Target.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.module.css
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.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/EmptyTextField.module.css
- frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.test.tsx
🧰 Additional context used
📓 Learnings (3)
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.
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/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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (6)
frontend/language/src/nb.json (1)
1470-1478: LGTM! Translation keys are well-organized and descriptive.The new translation keys for Summary2 override configuration are logically grouped and provide clear, user-friendly Norwegian translations.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.test.tsx (3)
9-47: LGTM! Comprehensive test coverage for EmptyTextField component.The test suite effectively covers all critical interactions:
- Button click to show text field
- Text input handling
- Editor closing behaviors (blur and Enter key)
- Proper onChange callback verification
The test cases are well-structured with clear descriptions.
49-56: LGTM! Well-structured helper functions for element queries.The helper functions use proper ARIA roles and i18n keys for querying elements, making the tests more maintainable and resilient to UI changes.
60-68: LGTM! Clean mock setup with proper typing.The render helper function is well-structured with:
- Proper typing for the override config
- Clear mock data setup
- Consistent use of the renderWithProviders utility
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (2)
24-32: LGTM! Well-structured props interface.The props interface is clear and properly typed, with new props for managing the open/close state of the override entry.
57-70: LGTM! Proper implementation of collapsed state.The collapsed state implementation follows the learned pattern for StudioProperty.Button, correctly handling undefined componentNameType cases.
...editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx
Show resolved
Hide resolved
...editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx
Outdated
Show resolved
Hide resolved
Ok that makes sense, i see where my confusion happened now. summary2-disable-save-button.mp4(not visible in the video, but the 'new override' button is no longer disabled like my previous changes)
This is only applicable to group components, so that's likely why you didn't see it. Selected a group component in the video above. |
|
NICE! LGTM 🥳 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
37-39: Consider simplifying the index manipulation logic.The index manipulation in
setOpenOverridescould be simplified for better readability while maintaining the same functionality.Consider this alternative:
- setOpenOverrides((prev) => { - return prev.filter((i) => i !== index).map((i) => (i > index ? i - 1 : i)); - }); + setOpenOverrides((prev) => prev + .filter((i) => i !== index) + .map((i) => i > index ? i - 1 : i) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (2)
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/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 (4)
- GitHub Check: CodeQL
- GitHub Check: Typechecking and linting
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (8)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (3)
Line range hint
1-13: LGTM! Clean imports and well-typed props.The imports are well-organized, and the type definitions are clear and properly typed.
46-46: Address spacing inconsistency mentioned in UX feedback.Based on the PR feedback from Annikenkbrathen, there's excessive space between buttons in the interface.
64-66: Update button styling to match UX guidelines.According to Annikenkbrathen's feedback, the button should appear as a blue "Add" button instead of black text.
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (5)
1-32: LGTM! Well-organized imports and clear type definitions.The imports are logically grouped and the
Summary2OverrideEntryPropstype is well-defined with the new properties supporting collapsible functionality.
Line range hint
34-56: LGTM! Clean component initialization with proper hooks usage.The component initialization is well-structured with appropriate use of hooks and efficient data transformation.
57-70: LGTM! Collapsed view follows established patterns.The implementation correctly handles undefined values as per the established pattern in the codebase, providing a clean and informative collapsed view.
73-109: LGTM! Well-structured expanded view with proper validation.The expanded view implementation is clean with appropriate conditional rendering and validation. The save button is correctly disabled when no component is selected, preventing empty overrides.
96-107: Verify button group styling.The button group uses CSS classes for styling. Please ensure the spacing between buttons matches the design requirements mentioned in the PR feedback.
Run this script to check the CSS implementation:
✅ Verification successful
Button group spacing implementation is correct
The implementation uses the standard design system spacing variable
--fds-spacing-3which is consistently used across the codebase for button groups and follows the established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the button group styling implementation cat frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.module.css | grep -A 5 "buttongroup"Length of output: 218
Script:
#!/bin/bash # Check for FDS spacing variables and global button group styles rg --type css "fds-spacing-3|buttongroup" -A 3Length of output: 40440
...editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx
Show resolved
Hide resolved
...editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx
Show resolved
Hide resolved
Description
Screen.Recording.2025-01-14.141702.mp4
Warning
Deviation from UX design sketches:
Text supplied for empty fields can only be a raw string, so the text key lookup is not added.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes