fix: summary2 set correct layoutset as default #14473
Conversation
|
Warning Rate limit exceeded@Jondyr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces comprehensive changes to the Summary2 component in the UX editor, focusing on enhancing configuration options for component summaries. The modifications span multiple files across the frontend packages, introducing new components, utility functions, and UI elements that improve the handling of component overrides, target selection, and display settings. The changes primarily aim to provide more granular control over how summary components are rendered and configured. Changes
Possibly related issues
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 (
|
Jondyr
changed the base branch from
main
to
21-01-summary2-filter-override-components
github-actions
bot
added
area/ui-editor
Jondyr
force-pushed
the
21-01-summary2-filter-override-components
branch
from
aee4443 to
0192d05
Compare
Jondyr
force-pushed
the
21-01-summary2-set-correct-layoutset-as-default
branch
from
5622521 to
95135d0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Nitpick comments (13)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.module.css (3)
1-9: Consider adding gap fallback for better browser compatibility.While the styles look good, consider adding a fallback for the
gapproperty to support older browsers..card { margin-top: var(--fds-spacing-4); margin-bottom: var(--fds-spacing-4); + /* Fallback for browsers that don't support gap */ + margin-bottom: calc(var(--fds-spacing-4) + var(--fds-spacing-4)); gap: var(--fds-spacing-4); }
11-13: Simplify property class by removing unnecessary padding.The
--fds-spacing-0variable likely equals 0, making this padding declaration unnecessary..property { - padding-left: var(--fds-spacing-0); }
26-29: Consider adding gap fallback for button group.While the flexbox layout is good, consider adding a fallback for the
gapproperty to support older browsers..buttongroup { display: flex; + /* Fallback for browsers that don't support gap */ + margin-right: calc(-1 * var(--fds-spacing-4)); + > * { + margin-right: var(--fds-spacing-4); + } gap: var(--fds-spacing-4); }frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ForceShowSwitch.tsx (2)
6-9: Align type name with component name for consistency.The props type name
ForceShowSwitchPropsdoesn't match the exported component nameOverrideShowComponentSwitch. Consider renaming the type toOverrideShowComponentSwitchPropsfor better consistency.-type ForceShowSwitchProps = { +type OverrideShowComponentSwitchProps = { onChange: (updatedOverride: Summary2OverrideConfig) => void; override: Summary2OverrideConfig; }; -export const OverrideShowComponentSwitch = ({ onChange, override }: ForceShowSwitchProps) => { +export const OverrideShowComponentSwitch = ({ onChange, override }: OverrideShowComponentSwitchProps) => {Also applies to: 11-11
21-21: Remove unnecessary value prop.The
valueprop onStudioSwitchappears to be unnecessary as it's not being used for any functional purpose. Consider removing it for cleaner code.<StudioSwitch position='right' size='sm' onChange={(event: React.ChangeEvent<HTMLInputElement>) => onChange({ ...override, hidden: !event.target.checked }) } checked={!override.hidden} - value={'hidden'} >frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/CompactViewSwitch.tsx (1)
22-26: Consider performance and type safety improvements.A few suggestions to enhance the code:
- Memoize the components array to prevent unnecessary recalculations
- Add null check before accessing component type
- Improve type safety by avoiding type assertion
+ const components = useMemo( + () => Object.values(formLayoutsData).flatMap((layout) => + getAllLayoutComponents(layout) + ), + [formLayoutsData] + ); - const components = Object.values(formLayoutsData).flatMap((layout) => - getAllLayoutComponents(layout), - ); const component = components.find((comp) => comp.id === override.componentId); - const isGroupComponent = component?.type === (ComponentType.Group as ComponentType); + const isGroupComponent = component?.type === ComponentType.Group;frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/targetUtils.ts (1)
84-86: Add null safety checks to getLayoutSetOptions.While the type change to
LayoutSetsModelimproves type safety, the function should handle potential null/undefined values more robustly.Consider this safer implementation:
export const getLayoutSetOptions = (layoutSets: LayoutSetsModel): LayoutSetModel[] => { - return layoutSets?.sets.filter((set) => set.task); + if (!layoutSets?.sets?.length) { + return []; + } + return layoutSets.sets.filter((set) => set?.task); };frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/ShowEmptyFieldsSwitch.tsx (1)
17-24: Consider removing unnecessary async.The event handler is marked as async but doesn't use any await operations.
- onChange={async (event: React.ChangeEvent<HTMLInputElement>) => { + onChange={(event: React.ChangeEvent<HTMLInputElement>) => {frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/EmptyTextField.tsx (1)
22-40: Consider adding aria-label for better accessibility.The StudioToggleableTextfield implementation looks good, but could benefit from accessibility improvements.
<StudioToggleableTextfield inputProps={{ icon: '', label: t('ux_editor.component_properties.summary.override.empty_field_text'), + 'aria-label': t('ux_editor.component_properties.summary.override.empty_field_text'), size: 'sm', value: override.emptyFieldText ?? '',frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/OverrideFields/Summary2OverrideDisplayType.tsx (1)
29-35: Consider extracting the component type check to a utility function.The component type check could be moved to a reusable utility function for better maintainability.
+const isCheckboxOrMultiselect = (componentId: string): boolean => + componentId.includes(ComponentType.MultipleSelect) || + componentId.includes(ComponentType.Checkboxes); export const Summary2OverrideDisplayType = ({ override, onChange, }: Summary2OverrideDisplayTypeProps) => { const { t } = useTranslation(); - const checkboxOrMultipleselect = - override.componentId.includes(ComponentType.MultipleSelect) || - override.componentId.includes(ComponentType.Checkboxes); + const checkboxOrMultipleselect = isCheckboxOrMultiselect(override.componentId);frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx (2)
25-26: Consider using a simpler boolean state for single accordion.Since there's only one accordion item, using a Record for the state might be unnecessary.
- const [accordionOpen, setAccordionOpen] = React.useState<Record<string, boolean>>({}); + const [isAccordionOpen, setIsAccordionOpen] = React.useState(false);
51-70: Simplify accordion state management.With the suggested boolean state, the accordion implementation can be simplified.
- <Accordion.Item open={accordionOpen['summary2overrides'] === true}> + <Accordion.Item open={isAccordionOpen}> <Accordion.Header - onHeaderClick={() => - setAccordionOpen((prev) => { - return { ...prev, summary2overrides: !prev['summary2overrides'] }; - }) - } + onHeaderClick={() => setIsAccordionOpen((prev) => !prev)} >frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2Override.tsx (1)
83-108: Improve type safety of useTargetComponentOptions hook.The hook returns
any[]which loses type safety. Consider creating a proper return type for better type checking and documentation.-const useTargetComponentOptions = (target: Summary2TargetConfig): any[] => { +type ComponentOption = { + id: string; + title: string; +}; +const useTargetComponentOptions = (target: Summary2TargetConfig): ComponentOption[] => {
🛑 Comments failed to post (4)
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2ComponentReferenceSelector.tsx (1)
33-35: 💡 Codebase verification
Missing English translation for empty state message
The translation key
ux_editor.component_properties.target_emptyis only present in the Norwegian translation file (nb.json) but missing in the English translation file (en.json). This needs to be added to ensure proper internationalization support.
- Add the translation key to
frontend/language/src/en.json🔗 Analysis chain
Verify the translation key exists.
Good addition of the empty state message using StudioCombobox.Empty. This improves user experience by providing clear feedback when no options are available.
Let's verify the translation key exists:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the translation key exists in the translation files # Expected result: The key 'ux_editor.component_properties.target_empty' should be present # Search for the translation key in JSON/TS files rg -g '*.{json,ts}' '"ux_editor\.component_properties\.target_empty"'Length of output: 171
Script:
#!/bin/bash # Search for translation files and then grep for our key fd -e json -p 'language/src/.*\.json$' --exec grep -l "ux_editor.component_properties.target_empty" {} \;Length of output: 140
Script:
#!/bin/bash # List all translation files to confirm available languages echo "Available language files:" fd -e json -p 'language/src/.*\.json$' echo -e "\nChecking en.json content for the key:" cat frontend/language/src/en.json | grep -A 1 "ux_editor\.component_properties\.target_empty"Length of output: 432
frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Summary2Target/targetUtils.ts (1)
43-52:
⚠️ Potential issueAdd error handling for layout set resolution.
The function has potential runtime issues:
- The
find()operation on line 49 could returnundefined, leading to a runtime error when accessing.id- No error handling for cases when the target task ID doesn't match any layout set
Consider this safer implementation:
export const getTargetLayoutSetName = ({ target, layoutSets, selectedFormLayoutSetName, }: getTargetLayoutSetNameProps): string => { - const layoutSetName = target?.taskId - ? layoutSets.sets.find((layoutSet) => layoutSet.task?.id === target.taskId).id - : selectedFormLayoutSetName; - return layoutSetName; + if (!target?.taskId) { + return selectedFormLayoutSetName; + } + const layoutSet = layoutSets.sets.find((set) => set.task?.id === target.taskId); + if (!layoutSet) { + console.warn(`Layout set not found for task ID: ${target.taskId}`); + return selectedFormLayoutSetName; + } + return layoutSet.id; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const getTargetLayoutSetName = ({ target, layoutSets, selectedFormLayoutSetName, }: getTargetLayoutSetNameProps): string => { if (!target?.taskId) { return selectedFormLayoutSetName; } const layoutSet = layoutSets.sets.find((set) => set.task?.id === target.taskId); if (!layoutSet) { console.warn(`Layout set not found for task ID: ${target.taskId}`); return selectedFormLayoutSetName; } return layoutSet.id; };frontend/packages/ux-editor/src/components/config/componentSpecificContent/Summary2/Override/Summary2OverrideEntry.tsx (1)
53-53: 🛠️ Refactor suggestion
Ensure
valueprop isfalsewhencomponentNameTypeis undefinedWhen
componentNameTypeis undefined, thevalueprop forStudioProperty.Buttonshould be set tofalseto maintain consistent behavior with theStudioProperty.Buttoncomponent.Apply this diff to adjust the
valueprop:- value={componentNameType && `${componentNameType} (ID:${override.componentId})`} + value={componentNameType ? `${componentNameType} (ID:${override.componentId})` : false}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.value={componentNameType ? `${componentNameType} (ID:${override.componentId})` : false}frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.test.tsx (1)
59-64:
⚠️ Potential issueRemove .only() from the test case.
The test case is currently focused using
it.only(), which prevents other tests from running. This is typically used during development and should be removed before merging.- it.only('should call handleComponentChange when changing target', async () => { + it('should call handleComponentChange when changing target', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.it('should call handleComponentChange when changing target', async () => { const user = userEvent.setup(); render(summary2Component); await user.selectOptions(summary2TargetLayoutSet(), layoutSet2NameMock); expect(handleComponentChange).toHaveBeenCalledTimes(1); });🧰 Tools
🪛 Biome (1.9.4)
[error] 59-59: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14473 +/- ##
=======================================
Coverage 95.70% 95.70%
=======================================
Files 1902 1902
Lines 24771 24771
Branches 2839 2839
=======================================
Hits 23707 23707
Misses 803 803
Partials 261 261 ☔ View full report in Codecov by Sentry. |
linting
Filters by applicable components I.e. If target is a layoutset, show only components from that layoutset
Jondyr
force-pushed
the
21-01-summary2-filter-override-components
branch
from
31fee1d to
a27a6e4
Compare
Jondyr
force-pushed
the
21-01-summary2-set-correct-layoutset-as-default
branch
from
bf55a30 to
f47bdec
Compare
Jondyr
force-pushed
the
21-01-summary2-set-correct-layoutset-as-default
branch
from
f47bdec to
53b0e0f
Compare
Jondyr
force-pushed
the
21-01-summary2-set-correct-layoutset-as-default
branch
2 times, most recently
from
c5def19 to
1bd1b19
Compare
Jondyr
force-pushed
the
21-01-summary2-set-correct-layoutset-as-default
branch
from
1bd1b19 to
e9cdf5b
Compare
Jondyr
force-pushed
the
21-01-summary2-set-correct-layoutset-as-default
branch
from
e9cdf5b to
8e89ed4
Compare
Description
layoutSet target is now selected layoutSet when adding a new summary2 component. Previously it was automatically set to the first layout set created.
PR stack
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
UI/UX Updates