feat: 14164 implement the legg til ny component for the last four standard choices #14392
Conversation
…egg-til-ny-component-for-the-last-four-standard-choices
github-actions
bot
added
area/ui-editor
JamalAlabdullah
changed the title
There was a problem hiding this comment.
Nice work! I tested it locally to see how it works and noticed some differences with the new design:
- The properties are not under the
Andre Instillingersection
- Should the other properties (e.g.,
Variant,HTML autofullfør, etc) not use the newCollapsiblePropertyEditorcomponent?
- I'd suggest showing the value instead of the + button when the property has a value, similar to how we handled text resources, data model bindings, etc
3.mov
| <Heading level={3} size='xxsmall'> | ||
| {t('ux_editor.component_properties.grid')} | ||
| </Heading> | ||
| <TextResource |
There was a problem hiding this comment.
Is there a reason for using the TextResource component here instead of the CollapsiblePropertyEditor component? It feels a bit strange to use the TextResource component here to display something that isn’t a text resource. Maybe we need to rename the TextResource component, I'm not sure
There was a problem hiding this comment.
I removed TextResource, and used Studio.card after i get ok from UX.
…egg-til-ny-component-for-the-last-four-standard-choices
📝 WalkthroughWalkthroughThis pull request introduces a new 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/packages/ux-editor/src/components/TextResource/TextResource.tsx (1)
20-22: 🛠️ Refactor suggestionConsider renaming the TextResource component.
The addition of
disableEditor,children, andclassNameprops suggests this component has evolved beyond just text resource handling. Consider renaming it to better reflect its current functionality, such asCollapsiblePropertyFieldorEditablePropertyField.
🧹 Nitpick comments (6)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.module.css (1)
16-18: Consider enhancing the gridButton class documentation and consistency.The implementation is clean and focused. Consider these improvements:
- Add a comment explaining the specific use case for zero padding
- Use CSS custom properties for consistency with other styles (e.g.,
padding: var(--fds-spacing-0))+/* Remove padding for grid-related buttons in CollapsiblePropertyEditor */ .gridButton { - padding: 0; + padding: var(--fds-spacing-0); }frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx (1)
36-41: Add keyboard support for closing the editor.Enhance user experience by adding keyboard support (Escape key) for closing the editor.
+ useEffect(() => { + const handleEscape = (event: KeyboardEvent) => { + if (event.key === 'Escape' && isVisible) { + setIsVisible(false); + } + }; + document.addEventListener('keydown', handleEscape); + return () => document.removeEventListener('keydown', handleEscape); + }, [isVisible]); <StudioButton icon={<XMarkIcon />} onClick={() => setIsVisible(false)} title={t('general.close')} variant='secondary' + aria-label={t('general.close')} />frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.test.tsx (1)
16-47: Expand test coverage for accessibility and keyboard interactions.Add test cases to verify:
- ARIA attributes presence and values
- Keyboard interaction (Escape key) behavior
- Focus management
describe('CollapsiblePropertyEditor', () => { + it('should have correct ARIA attributes', () => { + renderCollapsiblePropertyEditor({ label: 'Test label' }); + const button = screen.getByRole('button', { name: 'Test label' }); + expect(button).toHaveAttribute('aria-expanded', 'false'); + expect(button).toHaveAttribute('aria-controls', 'Test label-content'); + }); + + it('should close on Escape key press', async () => { + const user = userEvent.setup(); + renderCollapsiblePropertyEditor(); + await user.click(screen.getByText('Test label')); + expect(screen.getByText('Test children')).toBeInTheDocument(); + await user.keyboard('{Escape}'); + expect(screen.queryByText('Test children')).not.toBeInTheDocument(); + });frontend/packages/ux-editor/src/components/TextResource/TextResource.tsx (1)
147-154: Separate delete button visibility control from editor visibility.The
showTextResourceEditorprop controls both the editor and delete button visibility. Consider splitting this into separate concerns for better flexibility and maintainability.- showTextResourceEditor?: boolean; + showEditor?: boolean; + showDeleteButton?: boolean; {showTextResourceEditor && ( <StudioDeleteButton confirmMessage={t('ux_editor.text_resource_bindings.delete_confirm_question')} disabled={!onDelete} onDelete={() => onDelete?.()} title={t('general.delete')} /> )}frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (2)
136-152: Extract property-specific rendering logic into separate components.The conditional rendering logic for different property types (sortOrder, showValidations, preselectedOptionIndex) should be extracted into separate components for better maintainability.
Example for sortOrder:
const SortOrderProperty = ({ component, handleComponentUpdate }) => ( <CollapsiblePropertyEditor label={t('ux_editor.component_properties.sortOrder')} > <EditStringValue component={component} handleComponentChange={handleComponentUpdate} propertyKey="sortOrder" /> </CollapsiblePropertyEditor> );
136-196: Define constants for property keys.Replace hardcoded property key strings with constants to improve maintainability and reduce the risk of typos.
const PROPERTY_KEYS = { SORT_ORDER: 'sortOrder', SHOW_VALIDATIONS: 'showValidations', PRESELECTED_OPTION_INDEX: 'preselectedOptionIndex', } as const; // Usage const isSortOrder = propertyKey === PROPERTY_KEYS.SORT_ORDER;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/language/src/nb.json(2 hunks)frontend/packages/ux-editor/src/components/TextResource/TextResource.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/TextResource/TextResource.tsx(7 hunks)frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/index.ts(1 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.module.css
- frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/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 (4)
frontend/packages/ux-editor/src/components/TextResource/TextResource.test.tsx (2)
227-232: LGTM! Well-structured test cases for disableEditor prop.The test cases properly verify the TextResourceEditor's visibility based on the disableEditor prop value.
Also applies to: 234-239
255-269: LGTM! Good test coverage for delete button visibility.The test cases properly verify the delete button's visibility based on the disableEditor prop value.
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx (1)
163-243: LGTM! Comprehensive test coverage for new properties.The test cases thoroughly verify the rendering of CollapsiblePropertyEditor for:
- sortOrder property with combobox validation
- showValidations property with proper component rendering
- preselectedOptionIndex property with button presence check
frontend/language/src/nb.json (1)
1394-1394: LGTM! Clear and descriptive translations added.The Norwegian translations for the new properties are well-written and consistent with the existing translation style:
- preselectedOptionIndex_button: "Plassering av forhåndsvalgt verdi (indeks)"
- showValidations: "Vis valideringstyper"
Also applies to: 1423-1423
...ages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/TextResource/TextResource.test.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14392 +/- ##
=======================================
Coverage 95.68% 95.69%
=======================================
Files 1891 1893 +2
Lines 24588 24616 +28
Branches 2822 2830 +8
=======================================
+ Hits 23528 23556 +28
Misses 799 799
Partials 261 261 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (1)
156-167: 🛠️ Refactor suggestionConsider using CollapsiblePropertyEditor instead of TextResource.
Using TextResource for grid display seems inconsistent with its primary purpose, as noted in a previous review. The CollapsiblePropertyEditor would be more semantically appropriate here since:
- It's specifically designed for property editing
- It maintains consistency with how other properties are handled in this file
- It better represents the architectural intent
- <TextResource - handleIdChange={() => {}} - label={t('ux_editor.component_properties.grid')} - disableEditor - className={classes.gridButton} - > + <CollapsiblePropertyEditor + label={t('ux_editor.component_properties.grid')} + > <EditGrid key={component.id} component={component} handleComponentChange={handleComponentUpdate} /> - </TextResource> + </CollapsiblePropertyEditor>
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (3)
171-187: Simplify helpText logic for sortOrder.The helpText logic can be simplified by removing the redundant condition since it's only used for sortOrder.
<EditStringValue component={component} handleComponentChange={handleComponentUpdate} propertyKey={propertyKey} - helpText={isSortOrder ? '' : properties[propertyKey]?.description} + helpText={''} enumValues={properties[propertyKey]?.enum || properties[propertyKey]?.examples} />
207-215: Remove redundant key prop.The key prop is redundant here since it's already provided on the parent CollapsiblePropertyEditor.
<EditStringValue component={component} handleComponentChange={handleComponentUpdate} propertyKey={propertyKey} - key={propertyKey} helpText={isShowValidations ? '' : properties[propertyKey]?.description} enumValues={properties[propertyKey]?.items?.enum} multiple={true} />
231-246: Consider moving help text to translation files.The help text for preselectedOptionIndex is hardcoded. Consider moving it to the translation files for consistency and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(4 hunks)
⏰ 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 (2)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (2)
21-22: LGTM! New component imports.The addition of TextResource and CollapsiblePropertyEditor imports aligns with the PR objectives of enhancing the UI configuration.
257-284: LGTM! Proper state cleanup implementation.The implementation correctly handles the cleanup of validFileEndings when hasCustomFileEndings is disabled, preventing orphaned state.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx (1)
24-48: 🛠️ Refactor suggestionEnhance accessibility for screen readers.
The component needs proper ARIA attributes for better accessibility support.
<div className={cn(isVisible ? classes.collapsibleContainer : classes.collapsibleContainerClosed)} + role="region" + aria-labelledby={`${label}-button`} > {!isVisible ? ( <StudioProperty.Button className={classes.button} icon={icon} onClick={() => setIsVisible(true)} property={label} + aria-expanded={false} + aria-controls={`${label}-content`} + id={`${label}-button`} /> ) : ( <> - <div className={classes.editorContent}>{children}</div> + <div id={`${label}-content`} className={classes.editorContent}>{children}</div>
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx (2)
8-13: Add JSDoc documentation for the type definition.Consider adding JSDoc documentation to improve code maintainability and IDE support.
+/** + * Props for the CollapsiblePropertyEditor component + */ export type CollapsiblePropertyEditorProps = { + /** The label text to display on the button when collapsed */ label?: string; + /** The content to display when expanded */ children?: React.ReactNode; + /** Custom icon to display on the button when collapsed */ icon?: React.ReactNode; + /** When true, hides the close button in expanded state */ disabledCloseButton?: boolean; };
15-20: Optimize component performance with React.memo.Since this component might be used multiple times in a form, consider wrapping it with React.memo to prevent unnecessary re-renders.
-export const CollapsiblePropertyEditor = ({ +export const CollapsiblePropertyEditor = React.memo(({ label, children, disabledCloseButton = false, icon = <PlusCircleIcon />, -}: CollapsiblePropertyEditorProps) => { +}: CollapsiblePropertyEditorProps) => { // component implementation -}; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CodeQL
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
🔇 Additional comments (1)
frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx (1)
15-50: Handle undefined label gracefully.The component should handle cases where the label prop is undefined to prevent accessibility attribute errors.
export const CollapsiblePropertyEditor = ({ label, children, disabledCloseButton = false, icon = <PlusCircleIcon />, }: CollapsiblePropertyEditorProps) => { const { t } = useTranslation(); const [isVisible, setIsVisible] = useState(false); + const buttonId = label ? `${label}-button` : undefined; + const contentId = label ? `${label}-content` : undefined; return ( <div className={cn(isVisible ? classes.collapsibleContainer : classes.collapsibleContainerClosed)} - role="region" - aria-labelledby={`${label}-button`} + {...(label && { + role: "region", + 'aria-labelledby': buttonId + })} >Let me verify the usage of this component to ensure the label prop is consistently provided:
✅ Verification successful
The suggested changes for handling undefined label are appropriate.
The component's type definition shows label is optional (
label?: string), but the current implementation assumes it exists when setting aria attributes. While current usage shows consistent label provision, the defensive coding approach suggested in the review is valuable for maintaining robust accessibility support.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if CollapsiblePropertyEditor is used without a label prop # This could lead to accessibility issues # Search for usage of CollapsiblePropertyEditor ast-grep --pattern 'CollapsiblePropertyEditor $$$' -f ts,tsx # Search specifically for instances without label prop ast-grep --pattern '<CollapsiblePropertyEditor $$$>$$$</CollapsiblePropertyEditor>' -f ts,tsxLength of output: 514
Script:
#!/bin/bash # Search for CollapsiblePropertyEditor usage patterns ast-grep --pattern '<CollapsiblePropertyEditor $$$' # Fallback to ripgrep for a broader search rg -U "CollapsiblePropertyEditor.*" -A 3 # Look for prop types definition to understand if label is optional rg -U "type CollapsiblePropertyEditorProps" -A 5Length of output: 14046
...ages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx
Show resolved
Hide resolved
...ages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (3)
169-186: Consider simplifying the helpText logic.The helpText logic can be simplified by using a more declarative approach.
- helpText={isLayout || isSortOrder ? '' : properties[propertyKey]?.description} + helpText={properties[propertyKey]?.description ?? ''}This change:
- Removes the need for explicit empty string checks
- Maintains the same functionality
- Is more maintainable
187-205: Consider consolidating similar helpText patterns.The helpText logic shows the same pattern as in string properties. Consider extracting this logic into a helper function.
+ const getHelpText = (propertyKey: string, properties: any) => + properties[propertyKey]?.description ?? ''; - helpText={isShowValidations ? '' : properties[propertyKey]?.description} + helpText={getHelpText(propertyKey, properties)}
245-272: Consider improving type safety for hasCustomFileEndings check.The component access could be more type-safe.
- {component['hasCustomFileEndings'] && ( + {Boolean(component.hasCustomFileEndings) && (This change:
- Improves type safety
- Makes the code more explicit
- Prevents potential undefined behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/language/src/nb.json(2 hunks)frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/language/src/nb.json
- frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.module.css
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Testing
- GitHub Check: CodeQL
- GitHub Check: Typechecking and linting
🔇 Additional comments (3)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (3)
21-22: LGTM! Clean import additions.The new imports are well-organized and properly typed.
155-165: LGTM! Clean grid property implementation.The grid property section is well-structured with proper conditional rendering and heading hierarchy.
219-234: LGTM! Well-structured number properties implementation.The implementation maintains consistency with other property types and makes good use of translations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.module.css (2)
20-22: Remove unnecessary comment.The comment
/* Optional: Adjust padding as needed */is redundant and should be removed as it doesn't provide meaningful information..gridHeader { - padding: 0; /* Optional: Adjust padding as needed */ + padding: 0; }
16-22: Consider consolidating similar padding resets.Both
.gridButtonand.gridHeaderhave the same padding reset. Consider consolidating these selectors for better maintainability.-.gridButton { - padding: 0; -} - -.gridHeader { - padding: 0; -} +.gridButton, +.gridHeader { + padding: 0; +}frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (4)
155-189: Remove unnecessary Fragment wrapper.The Fragment wrapper around the grid conditional rendering is unnecessary as it contains only one child element.
- {grid && ( - <> - {showGrid ? ( + {grid && ( + showGrid ? ( <StudioCard> {/* ... card content ... */} </StudioCard> ) : ( <StudioProperty.Button className={classes.gridButton} icon={<PlusCircleIcon />} onClick={() => setShowGrid(true)} property={t('ux_editor.component_properties.grid')} /> - )} - </> + ) )}🧰 Tools
🪛 Biome (1.9.4)
[error] 156-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
193-210: Consider extracting string property rendering logic.The string property rendering logic contains conditional checks for
sortOrderandlayout. Consider extracting this into a separate component or function for better maintainability and readability.const renderStringProperty = (propertyKey: string) => { const isSortOrder = propertyKey === 'sortOrder'; const isLayout = propertyKey === 'layout'; return ( <CollapsiblePropertyEditor key={propertyKey} label={t(`ux_editor.component_properties.${propertyKey}` as TranslationKey)} > <EditStringValue component={component} handleComponentChange={handleComponentUpdate} propertyKey={propertyKey} helpText={isLayout || isSortOrder ? '' : properties[propertyKey]?.description} enumValues={properties[propertyKey]?.enum || properties[propertyKey]?.examples} /> </CollapsiblePropertyEditor> ); };
276-285: Simplify component update logic.The component update logic in the custom file endings section can be simplified by using object spread operator more effectively.
-handleComponentChange={(updatedComponent: FormComponent) => { - if (!updatedComponent.hasCustomFileEndings) { - handleComponentUpdate({ - ...updatedComponent, - validFileEndings: undefined, - }); - return; - } - handleComponentUpdate(updatedComponent); -}} +handleComponentChange={(updatedComponent: FormComponent) => + handleComponentUpdate({ + ...updatedComponent, + validFileEndings: updatedComponent.hasCustomFileEndings + ? updatedComponent.validFileEndings + : undefined, + }) +}
46-46: Consider adding loading state for grid toggle.The grid toggle functionality might benefit from a loading state when the grid editor is being initialized, especially for larger forms.
const [showGrid, setShowGrid] = useState(false); +const [isGridLoading, setIsGridLoading] = useState(false); +const handleGridToggle = async () => { + setIsGridLoading(true); + try { + setShowGrid(prev => !prev); + } finally { + setIsGridLoading(false); + } +}; // In the render: <StudioButton icon={<XMarkIcon />} - onClick={() => setShowGrid(false)} + onClick={handleGridToggle} + disabled={isGridLoading} title={t('general.close')} variant='secondary' className={classes.button} />Also applies to: 157-170
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
[error] 156-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CodeQL
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
🔇 Additional comments (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (1)
224-225: Fix inconsistent help text handling.The
helpTextprop is set to empty string forshowValidationsbut uses the description from properties for other cases. This seems inconsistent with the pattern used in string properties.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (1)
82-86: Consider making the test more maintainable.The hardcoded string
'subform_table_column_title_test-layout-set-3'makes the test brittle and tightly coupled to a specific layout. Consider:
- Using a constant or deriving the expected value dynamically from the test layout.
- Adding a comment explaining the format of the header content.
+// Define the expected header content format as a constant +const EXPECTED_HEADER_FORMAT = `subform_table_column_title_${layoutSet3SubformNameMock}`; expect(onEditMock).toHaveBeenCalledWith({ ...mockTableColumn, - headerContent: 'subform_table_column_title_test-layout-set-3', + headerContent: EXPECTED_HEADER_FORMAT, cellContent: { query: cellContentQueryMock, default: cellContentDefaultMock, }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(1 hunks)
…r-the-last-four-standard-choices
…egg-til-ny-component-for-the-last-four-standard-choices
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (2)
155-189: Remove unnecessary Fragment wrapper.The Fragment (
<>...</>) wrapper is redundant as it contains only one child element. You can simplify the code by removing it.- {grid && ( - <> + {grid && ( {showGrid ? ( <StudioCard> {/* ... */} </StudioCard> ) : ( <StudioProperty.Button {/* ... */} /> )} - </> + )}🧰 Tools
🪛 Biome (1.9.4)
[error] 156-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
277-286: Consider simplifying the component update logic.The current implementation could be simplified by using object spread operator more effectively.
handleComponentChange={(updatedComponent: FormComponent) => { - if (!updatedComponent.hasCustomFileEndings) { - handleComponentUpdate({ - ...updatedComponent, - validFileEndings: undefined, - }); - return; - } - handleComponentUpdate(updatedComponent); + handleComponentUpdate({ + ...updatedComponent, + validFileEndings: updatedComponent.hasCustomFileEndings + ? updatedComponent.validFileEndings + : undefined, + }); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/language/src/nb.json(2 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/language/src/nb.json
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
[error] 156-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🪛 GitHub Check: Typechecking and linting
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
[failure] 205-205:
Type '{ component: FormItem; handleComponentChange: (component: FormItem, mutateOptions?: UpdateFormMutateOptions) => void; propertyKey: string; helpText: any; enumValues: any; }' is not assignable to type 'IntrinsicAttributes & EditStringValueProps'.
[failure] 224-224:
Type '{ component: FormItem; handleComponentChange: (component: FormItem, mutateOptions?: UpdateFormMutateOptions) => void; propertyKey: string; key: string; helpText: any; enumValues: any; multiple: true; }' is not assignable to type 'IntrinsicAttributes & EditStringValueProps'.
[failure] 235-235:
Type '{ component: FormItem; handleComponentChange: (component: FormItem, mutateOptions?: UpdateFormMutateOptions) => void; propertyKey: string; key: string; helpText: any; enumValues: any; multiple: true; }' is not assignable to type 'IntrinsicAttributes & EditStringValueProps'.
[failure] 255-255:
Type '{ component: FormItem; handleComponentChange: (component: FormItem, mutateOptions?: UpdateFormMutateOptions) => void; propertyKey: "preselectedOptionIndex"; key: string; enumValues: any; helpText: string; }' is not assignable to type 'IntrinsicAttributes & EditNumberValueProps<ComponentType, "preselectedOptionIndex">'.
[failure] 276-276:
Type '{ propertyKey: string; component: FormItem; defaultValue: any; multiple: boolean; handleComponentChange: (updatedComponent: FormComponent) => void; }' is not assignable to type 'IntrinsicAttributes & EditBooleanValueProps'.
[failure] 293-293:
Type '{ component: FormItem; handleComponentChange: (component: FormItem, mutateOptions?: UpdateFormMutateOptions) => void; propertyKey: string; helpText: any; }' is not assignable to type 'IntrinsicAttributes & EditStringValueProps'.
[failure] 293-293:
Cannot find name 'validFileEndings'.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (1)
19-22: LGTM! Well-organized imports and state management.The new imports are properly organized and the state management follows React hooks best practices.
Also applies to: 46-46
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/components/config/editModal/EditStringValue.tsx (1)
47-47: LGTM! Consistent help text fallback pattern.The help text fallback pattern matches EditNumberValue's implementation.
Consider improving type safety for propertyKey.
While the implementation is correct, consider using generic constraints for
propertyKeysimilar to EditNumberValue for better type safety.-export interface EditStringValueProps extends IGenericEditComponent { - propertyKey: string; +export interface EditStringValueProps<T extends ComponentType, K extends keyof FormItem<T> & string> + extends IGenericEditComponent<T> { + propertyKey: K; enumValues?: string[]; multiple?: boolean; helpText?: string; } -export const EditStringValue = ({ +export const EditStringValue = <T extends ComponentType, K extends keyof FormItem<T> & string>({ component, handleComponentChange, propertyKey, enumValues, multiple, helpText, -}: EditStringValueProps): ReactElement => { +}: EditStringValueProps<T, K>): ReactElement => {frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (2)
155-186: Consider adding aria-expanded for better accessibility.The grid visibility control implementation is clean and follows good UX practices. However, consider adding aria-expanded attribute to improve accessibility.
<StudioProperty.Button className={classes.gridButton} icon={<PlusCircleIcon />} onClick={() => setShowGrid(true)} + aria-expanded={false} property={t('ux_editor.component_properties.grid')} />
193-210: Simplify the helpText logic.The helpText logic can be simplified by using a more concise condition.
- helpText={isLayout || isSortOrder ? '' : properties[propertyKey]?.description} + helpText={(!isLayout && !isSortOrder) ? properties[propertyKey]?.description : ''}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(5 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditNumberValue.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditStringValue.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
🔇 Additional comments (6)
frontend/packages/ux-editor/src/components/config/editModal/EditNumberValue.tsx (3)
23-23: LGTM! Well-typed optional property addition.The
helpTextproperty is correctly typed as an optional string in the interface.
31-31: LGTM! Proper prop destructuring.The
helpTextprop is correctly destructured from the component props.
53-53: LGTM! Good fallback pattern for help text.The help text fallback pattern is well implemented, prioritizing component-specific help text before falling back to the generic help text.
frontend/packages/ux-editor/src/components/config/editModal/EditStringValue.tsx (2)
17-17: LGTM! Consistent with EditNumberValue implementation.The
helpTextproperty is correctly added as an optional string.
26-26: LGTM! Proper prop destructuring.The
helpTextprop is correctly destructured from the component props.frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (1)
19-22: LGTM! Clean implementation of new imports and state management.The new imports are well-organized, and the state management for grid visibility is properly implemented using the useState hook.
Also applies to: 46-46
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (1)
156-189: Remove unnecessary Fragment wrapper.The Fragment wrapper is redundant as it contains only conditional rendering.
- {grid && ( - <> + {grid && showGrid ? ( + <StudioCard> + <StudioCard.Header className={classes.gridHeader}> + <div className={classes.flexContainer}> + <Heading size='xs' className={classes.heading}> + {t('ux_editor.component_properties.grid')} + </Heading> + <StudioButton + icon={<XMarkIcon />} + onClick={() => setShowGrid(false)} + title={t('general.close')} + variant='secondary' + className={classes.button} + /> + </div> + </StudioCard.Header> + <StudioCard.Content> + <EditGrid + key={component.id} + component={component} + handleComponentChange={handleComponentUpdate} + /> + </StudioCard.Content> + </StudioCard> + ) : grid && ( + <StudioProperty.Button + className={classes.gridButton} + icon={<PlusCircleIcon />} + onClick={() => setShowGrid(true)} + property={t('ux_editor.component_properties.grid')} + /> + )} - {showGrid ? ( - <StudioCard> - <StudioCard.Header className={classes.gridHeader}> - <div className={classes.flexContainer}> - <Heading size='xs' className={classes.heading}> - {t('ux_editor.component_properties.grid')} - </Heading> - <StudioButton - icon={<XMarkIcon />} - onClick={() => setShowGrid(false)} - title={t('general.close')} - variant='secondary' - className={classes.button} - /> - </div> - </StudioCard.Header> - <StudioCard.Content> - <EditGrid - key={component.id} - component={component} - handleComponentChange={handleComponentUpdate} - /> - </StudioCard.Content> - </StudioCard> - ) : ( - <StudioProperty.Button - className={classes.gridButton} - icon={<PlusCircleIcon />} - onClick={() => setShowGrid(true)} - property={t('ux_editor.component_properties.grid')} - /> - )} - </> - )}🧰 Tools
🪛 Biome (1.9.4)
[error] 156-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
[error] 156-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 231-231: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 255-255: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
- GitHub Check: CodeQL
🔇 Additional comments (2)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (2)
231-232:⚠️ Potential issueAdd missing key prop to EditStringValue component.
When rendering components in an array map, each component needs a unique key prop for React's reconciliation process.
- <EditStringValue {...commonProps} /> + <EditStringValue key={propertyKey} {...commonProps} />Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 231-231: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
255-256:⚠️ Potential issueAdd missing key prop to EditNumberValue component.
When rendering components in an array map, each component needs a unique key prop for React's reconciliation process.
- <EditNumberValue {...commonProps} /> + <EditNumberValue key={propertyKey} {...commonProps} />Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 255-255: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
|
When I try to check what Codecov is complaining about, I don't get any information. 
|
…r-the-last-four-standard-choices
…four-standard-choices' of https://github.com/Altinn/altinn-studio into 14164-implement-the-legg-til-ny-component-for-the-last-four-standard-choices
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Outdated
Show resolved
Hide resolved
| return isPreselectedOptionIndex ? ( | ||
| <CollapsiblePropertyEditor | ||
| key={propertyKey} | ||
| label={t('ux_editor.component_properties.preselectedOptionIndex_button')} | ||
| > | ||
| <EditNumberValue {...commonProps} /> | ||
| </CollapsiblePropertyEditor> | ||
| ) : ( | ||
| <EditNumberValue {...commonProps} /> | ||
| ); |
There was a problem hiding this comment.
I believe the CollapsiblePropertyEditor component should wrap all numbers
| return isPreselectedOptionIndex ? ( | |
| <CollapsiblePropertyEditor | |
| key={propertyKey} | |
| label={t('ux_editor.component_properties.preselectedOptionIndex_button')} | |
| > | |
| <EditNumberValue {...commonProps} /> | |
| </CollapsiblePropertyEditor> | |
| ) : ( | |
| <EditNumberValue {...commonProps} /> | |
| ); | |
| return ( | |
| <CollapsiblePropertyEditor | |
| key={propertyKey} | |
| label={t('ux_editor.component_properties.preselectedOptionIndex_button')} | |
| > | |
| <EditNumberValue {...commonProps} /> | |
| </CollapsiblePropertyEditor> | |
| ); |
There was a problem hiding this comment.
I have not fixed this yet, I am not sure , maybe if we will wrap all numbers with CollapsiblePropertyEditor we need to make the label :
label={t('ux_editor.component_properties.preselectedOptionIndex_button')} to
dynamic , something like this maybe? :
label={t(ux_editor.component_properties.${propertyKey}as TranslationKey)}
frontend/packages/ux-editor/src/components/config/editModal/EditNumberValue.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/editModal/EditNumberValue.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/editModal/EditNumberValue.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/editModal/EditStringValue.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/editModal/EditStringValue.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/editModal/EditStringValue.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (1)
156-189: Remove unnecessary Fragment.The Fragment wrapper is redundant since it contains only one conditional rendering block.
- {grid && ( - <> + {grid && ( {showGrid ? ( <StudioCard> {/* ... */} </StudioCard> ) : ( <StudioProperty.Button {/* ... */} /> )} - </> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 156-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
[error] 156-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 226-226: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 247-247: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ 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/FormComponentConfig.tsx (5)
Line range hint
19-46: LGTM: Clean imports and state management.The new imports and state management for grid visibility are well-organized and follow React best practices.
155-190: LGTM: Well-structured grid editing UI.The implementation provides a clean and intuitive interface for grid editing with proper state management and user feedback.
🧰 Tools
🪛 Biome (1.9.4)
[error] 156-189: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
191-206: LGTM: Clean string properties implementation.The string properties rendering is well-implemented with proper translation handling and consistent use of CollapsiblePropertyEditor.
226-226:⚠️ Potential issueAdd missing key prop to EditStringValue.
React requires a key prop when rendering elements in an iteration.
- <EditStringValue {...commonProps} /> + <EditStringValue key={propertyKey} {...commonProps} />Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 226-226: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
247-247:⚠️ Potential issueAdd missing key prop to EditNumberValue.
React requires a key prop when rendering elements in an iteration.
- <EditNumberValue {...commonProps} /> + <EditNumberValue key={propertyKey} {...commonProps} />Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 247-247: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
| return isPreselectedOptionIndex ? ( | ||
| <CollapsiblePropertyEditor | ||
| key={propertyKey} | ||
| label={t('ux_editor.component_properties.preselectedOptionIndex_button')} | ||
| > | ||
| <EditNumberValue {...commonProps} /> | ||
| </CollapsiblePropertyEditor> | ||
| ) : ( | ||
| <EditNumberValue {...commonProps} /> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap all number properties with CollapsiblePropertyEditor.
As suggested in past reviews, all number properties should be wrapped with CollapsiblePropertyEditor for consistency.
- return isPreselectedOptionIndex ? (
- <CollapsiblePropertyEditor
- key={propertyKey}
- label={t('ux_editor.component_properties.preselectedOptionIndex_button')}
- >
- <EditNumberValue {...commonProps} />
- </CollapsiblePropertyEditor>
- ) : (
- <EditNumberValue {...commonProps} />
- );
+ return (
+ <CollapsiblePropertyEditor
+ key={propertyKey}
+ label={t(`ux_editor.component_properties.${propertyKey}` as TranslationKey)}
+ >
+ <EditNumberValue {...commonProps} />
+ </CollapsiblePropertyEditor>
+ );📝 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.
| return isPreselectedOptionIndex ? ( | |
| <CollapsiblePropertyEditor | |
| key={propertyKey} | |
| label={t('ux_editor.component_properties.preselectedOptionIndex_button')} | |
| > | |
| <EditNumberValue {...commonProps} /> | |
| </CollapsiblePropertyEditor> | |
| ) : ( | |
| <EditNumberValue {...commonProps} /> | |
| ); | |
| return ( | |
| <CollapsiblePropertyEditor | |
| key={propertyKey} | |
| label={t(`ux_editor.component_properties.${propertyKey}` as TranslationKey)} | |
| > | |
| <EditNumberValue {...commonProps} /> | |
| </CollapsiblePropertyEditor> | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 247-247: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
Description
Based on our target to make UI for config/
andre stillingerin properties more readable, We added these properties to+ button: width(Grid), sorting, index and validation.CollapsiblePropertyEditor.CollapsiblePropertyEditor.NOTE:
Added to issues to:
Fix showing value from selector;
https://github.com/orgs/Altinn/projects/36/views/3?filterQuery=sprint%3A%40current+label%3A%22team%2Fstudio-domain2%22&pane=issue&itemId=94551591&issue=Altinn%7Caltinn-studio%7C14472
Modify som form;
https://github.com/orgs/Altinn/projects/36/views/3?filterQuery=sprint%3A%40current+label%3A%22team%2Fstudio-domain2%22&pane=issue&itemId=94559093&issue=Altinn%7Caltinn-studio%7C14474
Related Issue(s)
Before solution:
After solution:
Screen.Recording.2025-01-09.at.15.16.22.mov
Verification
Documentation
Summary by CodeRabbit
New Features
Documentation
Style
Tests