fix: 14472 should show the value of selector in properties config #14686
Conversation
github-actions
bot
added
area/ui-editor
📝 WalkthroughWalkthroughThe pull request introduces several changes, including the modification of a key-value pair in 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: 1
🧹 Nitpick comments (5)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.module.css (1)
5-8: Duplicate Declaration for.button
There are two separate declarations for the.buttonclass—with the first (lines 5–8) settingmargin: var(--fds-spacing-1)and the later one (lines 34–36) updating it tovar(--fds-spacing-2). Please confirm if this intentional dual declaration is desired for handling different contexts or if it might be beneficial to consolidate these styles into a single definition to improve maintainability and reduce potential specificity conflicts.Also applies to: 34-36
frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/SelectPropertyEditor.tsx (1)
15-47: Consider adding error boundaries and loading states.The component implementation is clean and follows React best practices. However, consider these improvements:
- Add error boundaries to handle potential rendering errors
- Add loading states for async operations
- Consider memoizing the callbacks to prevent unnecessary re-renders
export const SelectPropertyEditor = ({ children, value, property, title, }: SelectPropertyEditorProps) => { const { t } = useTranslation(); const [dataTypeSelectVisible, setDataTypeSelectVisible] = useState(false); + const handleClose = useCallback(() => setDataTypeSelectVisible(false), []); + const handleOpen = useCallback(() => setDataTypeSelectVisible(true), []); return ( <div className={cn(dataTypeSelectVisible ? classes.container : classes.viewMode)}> {dataTypeSelectVisible ? ( <div className={classes.editSelectProperty}> <div className={classes.selectProperty}>{children}</div> <StudioButton icon={<XMarkIcon />} - onClick={() => setDataTypeSelectVisible(false)} + onClick={handleClose} title={t('general.close')} variant='secondary' /> </div> ) : ( <StudioProperty.Button - onClick={() => setDataTypeSelectVisible(true)} + onClick={handleOpen} property={property} title={title} value={value} className={classes.viewSelectProperty} /> )} </div> ); };frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/SelectPropertyEditor.test.tsx (2)
12-38: Add more test coverage for edge cases.The test suite covers basic functionality well. Consider adding tests for:
- Error scenarios
- Empty/null values
- Long text overflow handling
- Keyboard navigation
- Accessibility (ARIA attributes)
46-48: Consider improving test helper function.The
renderSelectPropertyEditorhelper could be enhanced:const renderSelectPropertyEditor = (props: Partial<SelectPropertyEditorProps> = {}) => { - renderWithProviders()(<SelectPropertyEditor {...defaultProps} {...props} />); + const result = renderWithProviders()(<SelectPropertyEditor {...defaultProps} {...props} />); + return { + ...result, + rerender: (newProps: Partial<SelectPropertyEditorProps> = {}) => + result.rerender(<SelectPropertyEditor {...defaultProps} {...newProps} />), + }; };frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (1)
227-251: Consider extracting string property rendering logic.The string property handling logic is becoming complex. Consider extracting it into a separate component for better maintainability.
+const StringPropertyEditor = ({ propertyKey, component, handleComponentUpdate }) => { + const componentPropertyLabel = useComponentPropertyLabel(); + const selectedDataType = useComponentPropertyEnumValue(); + + const selectedStringPropertiesDisplay = () => { + const value = component[propertyKey]; + if (Array.isArray(value)) return value.map((dataType) => selectedDataType(dataType)); + if (value) return selectedDataType(value); + return undefined; + }; + + return ( + <SelectPropertyEditor + key={propertyKey} + property={componentPropertyLabel(propertyKey)} + title={componentPropertyLabel(propertyKey)} + value={selectedStringPropertiesDisplay()} + > + <EditStringValue + key={propertyKey} + component={component} + handleComponentChange={handleComponentUpdate} + propertyKey={propertyKey} + enumValues={properties[propertyKey]?.enum || properties[propertyKey]?.examples} + /> + </SelectPropertyEditor> + ); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/language/src/nb.json(1 hunks)frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.module.css(0 hunks)frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.test.tsx(0 hunks)frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx(0 hunks)frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/index.ts(0 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx(3 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(4 hunks)frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/SelectPropertyEditor.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/SelectPropertyEditor.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/SelectPropertyEditor.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/index.ts(1 hunks)
💤 Files with no reviewable changes (4)
- frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/index.ts
- frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.module.css
- frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.tsx
- frontend/packages/ux-editor/src/components/config/CollapsiblePropertyEditor/CollapsiblePropertyEditor.test.tsx
✅ Files skipped from review due to trivial changes (3)
- frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/index.ts
- frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/SelectPropertyEditor.module.css
- frontend/language/src/nb.json
⏰ 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.module.css (2)
15-18: New Style Addition for.gridButton
A new rule withmargin-bottom: var(--fds-spacing-0);has been added to the.gridButtonblock. This update appears intended to adjust the vertical spacing of grid-based buttons as per the new design specifications. Please verify that the chosen spacing is consistent with the overall UI guidelines.
34-36: Updated.buttonSpacing
The declaration for the.buttonclass has been modified to usemargin: var(--fds-spacing-2);. This change updates the spacing of button elements to align with new design requirements.frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/SelectPropertyEditor.tsx (1)
8-13: LGTM! Props interface is well-defined.The
SelectPropertyEditorPropsinterface is clean and properly typed with optional properties.frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (1)
21-22: LGTM! Good use of custom hooks.The addition of
useComponentPropertyEnumValuehook and state management is well-implemented.frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx (1)
163-242: LGTM! Test descriptions updated correctly.The test descriptions have been properly updated to reflect the new component structure, maintaining good test coverage.
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14686 +/- ##
==========================================
- Coverage 95.76% 95.76% -0.01%
==========================================
Files 1917 1916 -1
Lines 24979 24988 +9
Branches 2860 2859 -1
==========================================
+ Hits 23922 23930 +8
- Misses 798 799 +1
Partials 259 259 ☔ View full report in Codecov by Sentry. |
…onfig' of https://github.com/Altinn/altinn-studio into 14472-should-show-the-value-of-selector-in-properties-config
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx (1)
1-582: Consider adding more test cases for SelectPropertyEditor.While the current test coverage is good, consider adding these additional test cases for the
SelectPropertyEditor:
- Test for handling invalid/empty selections
- Test for proper state updates after selection
- Test for keyboard navigation
Example test case:
it('should handle keyboard navigation in SelectPropertyEditor', async () => { const user = userEvent.setup(); render({ props: { schema: { properties: { [selectedDataType]: { type: 'string', enum: ['option1', 'option2'], }, }, }, }, }); const button = screen.getByRole('button', { name: textMock(`ux_editor.component_properties.${selectedDataType}`), }); await user.click(button); const combobox = screen.getByRole('combobox', { name: textMock(`ux_editor.component_properties.${selectedDataType}`), }); // Test keyboard navigation await user.type(combobox, '{arrowdown}'); await user.type(combobox, '{enter}'); expect(combobox).toHaveValue('option1'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx(5 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
🔇 Additional comments (4)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx (4)
9-9: LGTM!The addition of
waitForfrom@testing-library/reactis appropriate for handling asynchronous test assertions.
16-16: LGTM!The constant
selectedDataTypeis well-named and aligns with its usage in the new test case.
164-189: Test descriptions accurately reflect the new component behavior.The test descriptions have been updated to reflect the removal of
CollapsiblePropertyEditorand now correctly describe the expected behavior of rendering property text. This aligns with the PR's objective of introducing the newSelectPropertyEditorcomponent.Also applies to: 191-219, 221-243
285-309: Test for SelectPropertyEditor follows best practices.The new test case:
- Properly sets up user events
- Verifies both the button and combobox rendering
- Uses appropriate assertions with
waitForfor async operations- Follows the same pattern as other similar tests in the file
This test is crucial for validating the fix for issue #14472, ensuring that selector values are properly displayed in the configuration panel.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx (1)
497-529: Good test coverage for enum values rendering.The test effectively verifies the rendering of enum values. Consider adding a test case for the empty state when no enum values are selected.
Add a test case for the initial state:
it('should render array properties with enum values correctly', async () => { const user = userEvent.setup(); const propertyKey = 'supportedArrayProperty'; const enumValues = ['option1', 'option2']; + // First test the empty state + render({ + props: { + schema: { + properties: { + [propertyKey]: { + type: 'array', + items: { + type: 'string', + enum: enumValues, + }, + }, + }, + }, + component: { + ...componentMocks.Input, + }, + }, + }); + const emptyArrayPropertyButton = screen.getByRole('button', { + name: textMock(`ux_editor.component_properties.${propertyKey}`), + }); + await user.click(emptyArrayPropertyButton); + expect(screen.getByRole('combobox')).toHaveValue(''); + + // Then test with selected values render({ props: { schema: {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx(6 hunks)
🔇 Additional comments (3)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx (3)
9-9: LGTM!Good addition of the
waitForutility for handling asynchronous operations in tests.
284-334: Well-structured test with comprehensive coverage!This test demonstrates several good practices:
- Proper setup of user events
- Clear test data preparation
- Step-by-step verification of component behavior
- Async/await usage for handling UI interactions
- Verification of both UI updates and callback invocations
163-242:✅ Verification successful
Test descriptions accurately reflect the new component structure.
The updated test descriptions align well with the PR's objective of replacing
CollapsiblePropertyEditorwithSelectPropertyEditor. The focus on testing functionality rather than specific component implementation is a good practice.Let's verify the test coverage for the new
SelectPropertyEditorcomponent:
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for SelectPropertyEditor # Look for test files related to SelectPropertyEditor fd "SelectPropertyEditor.*test" -t f # Look for test cases covering SelectPropertyEditor rg -A 5 "describe.*SelectPropertyEditor"Length of output: 1095
Updated Test Coverage and Component Structure Verification
The test descriptions now accurately reflect the new component structure. In addition to the FormComponentConfig tests, the new tests for the SelectPropertyEditor component (in
frontend/packages/ux-editor/src/components/config/SelectPropertyEditor/SelectPropertyEditor.test.tsx) confirm that the functionality is well-covered—e.g., verifying that children are rendered when the button is clicked. This aligns with the PR’s objective of replacing the CollapsiblePropertyEditor with the more focused SelectPropertyEditor, emphasizing proper functionality tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (3)
48-59: Optimize memoization for better performance.The current implementation of
memoizedGetSelectedValuesDisplaycould be optimized:
- Move the div creation logic outside the memoized function
- Consider memoizing the mapped values separately
Consider this optimization:
-const memoizedGetSelectedValuesDisplay = useMemo( - () => (propertyKey: string) => { - if (!component[propertyKey] || component[propertyKey].length === 0) return undefined; - return component[propertyKey].map((dataType: string) => ( - <div key={dataType}>{selectedDataType(dataType)}</div> - )); - }, - [component, selectedDataType], -); +const memoizedSelectedValues = useMemo( + () => (propertyKey: string) => { + if (!component[propertyKey] || component[propertyKey].length === 0) return undefined; + return component[propertyKey].map(selectedDataType); + }, + [component, selectedDataType], +); + +const getSelectedValuesDisplay = (propertyKey: string) => { + const values = memoizedSelectedValues(propertyKey); + return values?.map((value: string) => ( + <div key={value}>{value}</div> + )); +};
266-282: Add number formatting for better readability.Consider formatting the number value for better user experience.
Consider this improvement:
-value={component[propertyKey]} +value={typeof component[propertyKey] === 'number' ? + component[propertyKey].toLocaleString() : + component[propertyKey]}
300-307: Improve error message for better debugging.The error handling is good, but the error message could be more descriptive.
Consider this improvement:
-console.error(error); +console.error( + `Error updating component for property ${propertyKey}:`, + error, + { component: updatedComponent } +);
📜 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(5 hunks)
⏰ 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/FormComponentConfig.tsx (1)
1-1: LGTM! Clean import organization.The new imports are well-organized and follow React best practices.
Also applies to: 21-22
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 (3)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (3)
51-59: Add error handling to memoizedGetSelectedValuesDisplay.The function should handle potential errors when accessing component properties or during map operations.
const memoizedGetSelectedValuesDisplay = useMemo( () => (propertyKey: string) => { - if (!component[propertyKey] || component[propertyKey].length === 0) return undefined; - return component[propertyKey].map((dataType: string) => ( - <div key={dataType}>{selectedDataType(dataType)}</div> - )); + try { + if (!component[propertyKey] || component[propertyKey].length === 0) return undefined; + return component[propertyKey].map((dataType: string) => ( + <div key={dataType}>{selectedDataType(dataType)}</div> + )); + } catch (error) { + console.error('Error displaying selected values:', error); + return undefined; + } }, [component, selectedDataType], );
253-267: Remove duplicate key prop.The key prop is unnecessarily duplicated on both the SelectPropertyEditor and EditStringValue components.
<SelectPropertyEditor key={propertyKey} property={componentPropertyLabel(propertyKey)} title={componentPropertyLabel(propertyKey)} value={memoizedSelectedStringPropertiesDisplay(propertyKey)} > <EditStringValue - key={propertyKey} component={component} handleComponentChange={handleComponentUpdate} propertyKey={propertyKey} enumValues={properties[propertyKey]?.enum || properties[propertyKey]?.examples} /> </SelectPropertyEditor>
294-298: Memoize selectProperty calculation.The selectProperty calculation should be memoized to prevent unnecessary recalculations on each render.
+const selectProperty = useMemo( + () => + selectedValue.length > 0 + ? t('ux_editor.component_properties.selected_validations') + : componentPropertyLabel(propertyKey), + [selectedValue.length, t, componentPropertyLabel, propertyKey] +); -const selectProperty = - selectedValue.length > 0 - ? t('ux_editor.component_properties.selected_validations') - : componentPropertyLabel(propertyKey);
📜 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(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
🔇 Additional comments (2)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (2)
273-289: Remove duplicate key prop.The key prop is unnecessarily duplicated on both the SelectPropertyEditor and EditNumberValue components.
<SelectPropertyEditor key={propertyKey} property={componentPropertyLabel( `${propertyKey}${propertyKey === 'preselectedOptionIndex' ? '_button' : ''}`, )} title={componentPropertyLabel(propertyKey)} value={component[propertyKey]} > <EditNumberValue component={component} handleComponentChange={handleComponentUpdate} propertyKey={propertyKey} - key={propertyKey} enumValues={properties[propertyKey]?.enum} /> </SelectPropertyEditor>
299-317: Remove duplicate key prop.The key prop is unnecessarily duplicated on both the SelectPropertyEditor and EditStringValue components.
<SelectPropertyEditor key={propertyKey} property={selectProperty} title={componentPropertyLabel(propertyKey)} value={memoizedGetSelectedValuesDisplay(propertyKey)} > <EditStringValue component={component} handleComponentChange={(updatedComponent) => { setSelectedValue(updatedComponent[propertyKey]); handleComponentUpdate(updatedComponent); }} propertyKey={propertyKey} - key={propertyKey} enumValues={properties[propertyKey]?.items?.enum} multiple={true} /> </SelectPropertyEditor>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx (2)
336-369: Consider enhancing error handling test coverage.While the test verifies that
handleComponentUpdateis not called on error, it could be improved to:
- Test specific error scenarios (e.g., invalid enum values, type mismatches)
- Verify error messages or UI feedback
- Test error recovery paths
Example enhancement:
it('should return undefined when an error occurs in memoizedSelectedStringPropertiesDisplay', async () => { const propertyKey = 'invalidPropertyKey'; const handleComponentUpdateMock = jest.fn(); const user = userEvent.setup(); + const consoleSpy = jest.spyOn(console, 'error').mockImplementation(); render({ props: { schema: { properties: { [propertyKey]: { type: 'array', items: { - type: 'string', - enum: ['option1', 'option2'], + type: 'number', // Intentional type mismatch + enum: [1, 2], }, }, }, }, handleComponentUpdate: handleComponentUpdateMock, }, }); // ... existing test code ... + // Verify error handling + expect(consoleSpy).toHaveBeenCalled(); + expect(screen.getByText(/error message/i)).toBeInTheDocument(); + + // Test recovery + await user.click(screen.getByRole('button', { name: /close/i })); + expect(screen.queryByText(/error message/i)).not.toBeInTheDocument(); + + consoleSpy.mockRestore(); });
284-334: Consider adding edge cases to array property update test.The test effectively covers the happy path, but consider adding test cases for:
- Multiple selections
- Deselection
- Empty array handling
- Maximum selection limit (if applicable)
Example additions:
it('should call handleComponentUpdate and setSelectedValue when array property is updated', async () => { // ... existing test code ... + // Test multiple selections + const option2 = screen.getByRole('option', { + name: textMock('ux_editor.component_properties.enum_option2'), + }); + await user.click(option2); + + await waitFor(() => { + expect(handleComponentUpdateMock).toHaveBeenCalledWith( + expect.objectContaining({ + [propertyKey]: ['option1', 'option2'], + }), + ); + }); + + // Test deselection + await user.click(option1); + + await waitFor(() => { + expect(handleComponentUpdateMock).toHaveBeenCalledWith( + expect.objectContaining({ + [propertyKey]: ['option2'], + }), + ); + }); }); + +it('should handle empty array selection correctly', async () => { + // Add test for empty array handling +}); + +it('should respect maximum selection limit', async () => { + // Add test for maximum selection limit +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx(6 hunks)
⏰ 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 (2)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx (2)
9-9: LGTM! Import ofwaitForis necessary for async testing.The addition of
waitForfrom@testing-library/reactis appropriate for handling asynchronous operations in the new test cases.
163-242: LGTM! Test descriptions accurately reflect the new component behavior.The updated test cases properly verify the rendering of property text instead of checking for specific component types, which aligns with the PR's objective of replacing
CollapsiblePropertyEditorwithSelectPropertyEditor.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (2)
51-68: Add error handling to memoized functions.While the memoization is good for performance, consider adding error handling to gracefully handle potential undefined values or type mismatches.
const memoizedGetSelectedValuesDisplay = useMemo( () => (propertyKey: string) => { + try { if (!component[propertyKey] || component[propertyKey].length === 0) return undefined; return component[propertyKey].map((dataType: string) => ( <div key={dataType}>{selectedDataType(dataType)}</div> )); + } catch (error) { + console.error('Error displaying selected values:', error); + return undefined; + } }, [component, selectedDataType], ); const memoizedSelectedStringPropertiesDisplay = useMemo( () => (propertyKey: string) => { + try { const value = component[propertyKey]; if (Array.isArray(value)) return value.map((dataType) => selectedDataType(dataType)); return value ? selectedDataType(value) : undefined; + } catch (error) { + console.error('Error displaying string properties:', error); + return undefined; + } }, [component, selectedDataType], );
288-313: Improve state management in array properties.The state management for selected values could be improved to prevent potential race conditions.
-handleComponentChange={(updatedComponent) => { - setSelectedValue(updatedComponent[propertyKey]); - handleComponentUpdate(updatedComponent); -}} +handleComponentChange={useCallback((updatedComponent) => { + try { + const newValue = updatedComponent[propertyKey] || []; + setSelectedValue(newValue); + handleComponentUpdate(updatedComponent); + } catch (error) { + console.error('Error updating component:', error); + } +}, [propertyKey, handleComponentUpdate])}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx(6 hunks)frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx(5 hunks)
⏰ 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 (4)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (3)
1-1: LGTM! Well-structured hooks and imports.The new hooks and imports are well-organized and properly typed, supporting the PR's objective of displaying selector values.
Also applies to: 21-22, 48-49
246-263: LGTM! Clean implementation of SelectPropertyEditor.The replacement of CollapsiblePropertyEditor with SelectPropertyEditor is well-implemented, with proper property and title props.
266-285: LGTM! Consistent implementation for number properties.The SelectPropertyEditor implementation for number properties is consistent with string properties, with proper handling of preselectedOptionIndex.
frontend/packages/ux-editor/src/components/config/FormComponentConfig.test.tsx (1)
284-334: LGTM! Comprehensive test coverage.The new tests thoroughly cover:
- Array property updates and state management
- Enum value rendering
- User interactions with the SelectPropertyEditor
The test cases are well-structured and provide good coverage of the new functionality.
Also applies to: 486-518
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/language/src/nb.json(1 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
🪛 GitHub Check: CodeQL
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
[notice] 49-49: Unused variable, import, function or class
Unused variable selectedValue.
🪛 GitHub Check: Typechecking and linting
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
[failure] 49-49:
'selectedValue' is declared but its value is never read.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
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
…onfig' of https://github.com/Altinn/altinn-studio into 14472-should-show-the-value-of-selector-in-properties-config
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (3)
21-22: Add error handling for the enum value hook.Consider wrapping the hook usage in a try-catch block to handle potential errors gracefully.
- const selectedDataType = useComponentPropertyEnumValue(); + const selectedDataType = useMemo(() => { + try { + return useComponentPropertyEnumValue(); + } catch (error) { + console.error('Error getting enum value:', error); + return (value: string) => value; + } + }, []);Also applies to: 48-49
247-261: Optimize property and title props in SelectPropertyEditor.The
propertyandtitleprops are being set to the same value, which is redundant.<SelectPropertyEditor key={propertyKey} property={componentPropertyLabel(propertyKey)} - title={componentPropertyLabel(propertyKey)} value={memoizedSelectedStringPropertiesDisplay(propertyKey)} >
267-283: Simplify property label construction.Consider extracting the complex property label logic into a separate memoized function.
+const getPropertyLabel = useMemo( + () => (propertyKey: string): string => { + return componentPropertyLabel( + `${propertyKey}${propertyKey === 'preselectedOptionIndex' ? '_button' : ''}` + ); + }, + [componentPropertyLabel] +); <SelectPropertyEditor key={propertyKey} - property={componentPropertyLabel( - `${propertyKey}${propertyKey === 'preselectedOptionIndex' ? '_button' : ''}`, - )} + property={getPropertyLabel(propertyKey)} title={componentPropertyLabel(propertyKey)} value={component[propertyKey]} >
📜 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(5 hunks)
🔇 Additional comments (1)
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx (1)
289-305: LGTM! Array properties handling has been improved.The implementation correctly handles array properties and the unused state variable has been removed as suggested in past reviews.
frontend/packages/ux-editor/src/components/config/FormComponentConfig.tsx
Show resolved
Hide resolved
mlqn
deleted the
14472-should-show-the-value-of-selector-in-properties-config
branch
Description
In the config panel, we add all selectors inside the + button, but we missed displaying the selected value when the user chooses an option from the selector. This has been fixed in this PR.
Changes:
SelectPropertyEditor.CollapsiblePropertyEditorfolder and replaced it with the new component.Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Style
SelectPropertyEditorcomponent.Refactor
SelectPropertyEditorfor managing property displays.Bug Fixes
FormComponentConfigtests.Chores
CollapsiblePropertyEditor.