feat: Main section text and binding #14814
Conversation
📝 WalkthroughWalkthroughThis pull request introduces updates to the UX Editor package, including new components ( Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
area/ui-editor
lassopicasso
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14814 +/- ##
=======================================
Coverage 95.94% 95.95%
=======================================
Files 2012 2014 +2
Lines 26012 26038 +26
Branches 2936 2943 +7
=======================================
+ Hits 24958 24984 +26
Misses 788 788
Partials 266 266 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (2)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/TextMainConfig.tsx (1)
1-49: Well-structured component with clear responsibilitiesThe TextMainConfig component is well-designed, handling text resource bindings with clear separation of concerns. The conditional rendering based on titleKey presence is a good pattern for avoiding unnecessary rendering.
I noticed the static analysis flagged line 37 for using the delete operator. While this works, it could impact performance in some cases.
- const componentCopy = { ...component }; - delete componentCopy.textResourceBindings?.title; - handleComponentChange(componentCopy); + const { textResourceBindings, ...rest } = component; + const newTextResourceBindings = textResourceBindings + ? { ...textResourceBindings } + : {}; + if ('title' in newTextResourceBindings) { + delete newTextResourceBindings.title; + } + handleComponentChange({ + ...rest, + ...(Object.keys(newTextResourceBindings).length > 0 + ? { textResourceBindings: newTextResourceBindings } + : {}) + });Alternatively, a simpler approach could be:
- const componentCopy = { ...component }; - delete componentCopy.textResourceBindings?.title; - handleComponentChange(componentCopy); + const { textResourceBindings, ...rest } = component; + const updatedTextResourceBindings = { ...textResourceBindings }; + delete updatedTextResourceBindings.title; + + handleComponentChange({ + ...rest, + textResourceBindings: Object.keys(updatedTextResourceBindings).length > 0 + ? updatedTextResourceBindings + : undefined, + });🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx (1)
199-205: Consider adding a comment explaining theuseSetQueryDataSchemaparameterThe parameter addition is a good improvement to allow testing of loading states, but it would benefit from a brief documentation comment.
const renderPropertiesHeader = ( props: Partial<PropertiesHeaderProps> = {}, + // Controls whether to pre-populate component schema data + // Set to false to test loading states useSetQueryDataSchema: boolean = true, ) => { const componentType = props.formItem ? props.formItem.type : defaultProps.formItem.type;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/packages/ux-editor/src/components/Properties/Properties.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/DataModelMainConfig.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/DataModelMainConfig.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/HeaderMainConfig.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx(4 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/TextMainConfig.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/TextMainConfig.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/TextMainConfig.tsx
[error] 37-37: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
🔇 Additional comments (16)
frontend/packages/ux-editor/src/components/Properties/Properties.test.tsx (2)
17-17: Adding import for componentSchemaMocks is a good additionThis correctly imports the mocks needed for the component schema data in the tests.
301-304: Good enhancement to mock the component schema dataSetting up the query data for FormComponent ensures that the component schema is available during testing, which aligns with the changes in PropertiesHeader to support loading states and schema data.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/HeaderMainConfig.tsx (1)
7-7: Simplified component signature looks goodRemoving the children prop simplifies the component, making it more focused on its specific purpose of rendering the main configuration heading. This change aligns with the current implementation where the component doesn't need to render child elements.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/DataModelMainConfig.test.tsx (1)
1-81: Well-structured tests for the new DataModelMainConfig componentThe test suite is comprehensive and covers different scenarios effectively:
- Testing for null return when schema lacks data model binding
- Verifying correct rendering when dataModelBindings is defined
- Testing update functionality for deleting bindings
The helper function for rendering the component with necessary context is well implemented.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/ComponentMainConfig.tsx (2)
4-5: Good addition of specialized type importsImporting the specific FormComponent and FormContainer types will provide better type safety than using the more generic FormItem type.
8-9: Improved type definitions for better component handlingUpdating the component prop type to accept either FormComponent or FormContainer enhances the flexibility of this component, allowing it to handle a wider range of component types in the UX editor.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/DataModelMainConfig.tsx (1)
1-39: Clean implementation with good error handlingThe DataModelMainConfig component is well-implemented with proper error handling for missing dataModelBindingKey. The use of the useFormItemContext hook for accessing the debounceSave function shows good integration with the wider component ecosystem.
The conditional label logic at line 35 is particularly elegant, only showing the key name when it's not 'simpleBinding'.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/TextMainConfig.test.tsx (1)
1-107: Comprehensive test coverage with good practicesThe tests for the TextMainConfig component are well-structured and cover key functionality including:
- Rendering behavior with and without a title prop
- Text resource selection behavior
- Deletion of text resource binding
The use of helper functions like
textButton()andrenderTextMainConfig()improves readability and maintainability. The mock setup is clean and comprehensive.frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx (5)
5-5: Good inclusion of necessary componentsThe addition of StudioSpinner to the import list is appropriate for handling loading states.
16-18: Proper imports for new component integrationThe imports for useComponentSchemaQuery, TextMainConfig, and DataModelMainConfig are well-organized and necessary for the component enhancements.
30-42: Excellent loading state handlingAdding loading state management with the useComponentSchemaQuery hook and displaying a spinner during the loading phase significantly improves the user experience. The early return pattern for loading states is a clean approach.
The destructuring of schema properties is also well-implemented for clarity.
48-48: Simplified conditionThe hideMainConfig condition has been simplified for better readability, focusing on the essential condition.
75-101: Good integration of new components with feature flagsThe integration of TextMainConfig and DataModelMainConfig components with appropriate feature flag checking is well-implemented. The components receive the correct props from the parent component and schema.
The structure of conditional rendering based on the hideMainConfig variable is clear and logical.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx (3)
35-35: Good addition to cleanup between testsAdding
queryClientMock.clear()ensures that query data doesn't leak between test cases, which helps maintain test isolation and prevents unexpected behavior.
205-210: Good implementation of conditional query data settingThe conditional check allows tests to control whether component schema data is pre-loaded, which is essential for testing loading states.
213-214:✅ Verification successful
Simplified render output
Removing the FormItemContext.Provider wrapper simplifies the test setup. Ensure that the component still works correctly without this context if it was previously required.
🏁 Script executed:
#!/bin/bash # Check if FormItemContext is used in PropertiesHeader.tsx rg -A 3 -B 3 "FormItemContext" ./frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsxLength of output: 124
I'll run an additional script to search specifically for any usage of
FormItemContext.Providerin the affected file to further verify if its removal impacts the component's behavior.
🏁 Script executed:
#!/bin/bash # Check if FormItemContext.Provider is used in PropertiesHeader.tsx rg -n "FormItemContext\.Provider" ./frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsxLength of output: 129
Verified: Simplified render output is safe
The search confirmed that no instance ofFormItemContext.Providerexists inPropertiesHeader.tsx, which indicates that removing it from the test setup does not affect the component's actual usage. No further changes are needed on this front.
...tend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx
Outdated
Show resolved
Hide resolved
...d/packages/ux-editor/src/components/Properties/PropertiesHeader/DataModelMainConfig.test.tsx
Show resolved
Hide resolved
...d/packages/ux-editor/src/components/Properties/PropertiesHeader/DataModelMainConfig.test.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/DataModelMainConfig.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.test.tsx (1)
27-32: Consider expanding test coverageThe test is now only checking for the presence of the target text label. Consider adding assertions to verify that the actual target selection functionality works correctly, since that's a key part of the component's purpose.
it('should render summary2 config', async () => { render(summary2Component); expect( screen.getByText(textMock('ux_editor.component_properties.target')), ).toBeInTheDocument(); + // Verify target selection dropdown is present + expect(summary2TargetLayoutSet()).toBeInTheDocument(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/DataModelMainConfig.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx(4 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/DataModelMainConfig.test.tsx
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx
⏰ 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 (4)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/SpecificMainConfig/SummaryMainConfig.test.tsx (4)
11-12: Improved import structureThe import changes look good. Adding
textMockfrom the i18n mocks helps with text-based assertions, and importinglayoutSetsExtendedMockaligns with its usage in the query client setup.
29-31: Better assertion approach using i18n text mockGood change to check for the presence of the internationalized text rather than testing implementation details. This makes the tests more resilient to UI refactoring while still verifying the expected content is displayed.
49-50: Simplification of test setupThe removal of unnecessary mock data and setup code keeps the tests focused on what's being tested. The query client setup with just the layout sets data is sufficient for these tests.
53-53: Simplified render optionsThe simplification of the
renderWithProvidersoptions to only include what's needed (the query client) is a good practice. This reduces test complexity and maintenance overhead.
There was a problem hiding this comment.
Works well 👍 But i have some questions/concerns:
Since they are UX-related i'm adding @Annikenkbrathen @Ildest in case you have comments.
Some properties might need a new visualization when rendered outside their original context.
For example, in my opinion, it's not clear that this is a datamodel binding:
Maybe there should be a label or small subheading to indicate that this is a datamodel-binding? Or maybe something else can help to clarify this?
Just the first datamodelbinding is shown as required, which might be misleading
For example, Address just shows address, while zipCode and postalPlace are also required datamodel fields.
I don't think we have a good way to check which fields are required with todays solution(?), but i think maybe we should either show all required or none for these cases.
@Jondyr
Should maybe render all datamodelbindings regardless, when component have more than one, @Annikenkbrathen? |
|
Datamodel-bindings: Could we do that? Subtitles: My initial thought was to avoid titles altogether, so users would have to open the “Add” button to understand what it is. However, I’m unsure if that would be too confusing since they wouldn’t know what it is before clicking.
|
Or could we try add a tooltip with |
|
@Annikenkbrathen Quick thought for the datamodelbindings; Could we change the displayname of The default datamodel binding for a component just being the component name might be superfluous? That way we can avoid having a label/title and still communicate what the actual property is? 🤔 |
|
So you're thinking of only displaying And how does it look in the datamodel accordion further down? Does it stay the same as it is now? or does we rename them too? Is it weird to have different naming there and in the main config?🤔 |
I might be wrong, but i believe components with multiple bindings do not use
It makes sense to me to have this both places. It feels more consistent, with properties like "ledetekst" in my opinion. |
Description
This PR adds title config and main data model binding for the component to the main config, behind a feature flag. I have manually tested it with components such as a simple input, address, summary, and subform.
Refactored the code to make adding additional component specific configs to the main config easier.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Summary by CodeRabbit
New Features
DataModelMainConfigandTextMainConfigcomponents for enhanced management of form settings.PropertiesHeadercomponent to improve user feedback during data fetching.Summary2component type, enhancing its functionality.DataModelMainConfigcomponent and introduced tests for theTextMainConfigcomponent to improve coverage.Bug Fixes
Refactor
Style
.mainContentclass to improve layout spacing.Chores