feat: Subform table hide component selector when column has already one copied #14476
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant modifications to the 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 (
|
lassopicasso
changed the title
github-actions
bot
added
area/ui-editor
lassopicasso
changed the title
|
@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: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (1)
84-113: Consider grouping related test helper functions.The test helper functions are well-implemented, but consider moving
selectComponentSelectorcloser torenderEditColumnElementComponentSelectfor better code organization.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (1)
87-98: Consider a more robust condition check.While the implementation works, checking string length might be fragile. Consider using a more explicit condition:
- const isComponentCopySaved = sourceColumn.headerContent.length > 0; + const isComponentCopySaved = Boolean(sourceColumn.headerContent);This change:
- Handles undefined/null values safely
- Makes the intent clearer
- Is consistent with boolean conversions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(3 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 (5)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (3)
7-16: LGTM! Well-organized imports.The imports are properly structured and include all necessary testing utilities and component types.
17-32: LGTM! Well-structured test setup.The default props and test data are well-defined, providing a solid foundation for testing different scenarios.
34-58: LGTM! Tests effectively cover the component selector visibility logic.The test cases thoroughly verify the main PR objective:
- Component selector is shown when no component is selected
- Component selector is hidden when a component is already selected and saved
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (2)
Line range hint
27-41: LGTM! Clear and well-typed props interface.The renaming of
ColumnElementPropstoEditColumnElementPropsimproves clarity and better reflects the component's purpose.
93-98: LGTM! Clean implementation of component selector visibility.The conditional rendering logic effectively implements the PR objective of hiding the component selector when a column already has a copied component.
…olumn-has-one-copied
lassopicasso
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (1)
114-121: Consider enhancing the selectComponentSelector helper function.While the function works correctly, it could be more reusable by returning the componentSelector for chaining in tests.
const selectComponentSelector = async () => { const user = userEvent.setup(); const componentSelector = screen.getByRole('combobox', { name: textMock('ux_editor.properties_panel.subform_table_columns.choose_component'), }); expect(componentSelector).toBeInTheDocument(); await user.click(componentSelector); + return componentSelector; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx
🔇 Additional comments (3)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (1)
18-21: LGTM! Test mock data accurately reflects initial column state.The empty strings in mockTableColumn correctly represent the initial state of a new column before component selection.
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.test.tsx (1)
34-58: LGTM! Test coverage aligns perfectly with PR objective.The test cases comprehensively verify the component selector's visibility behavior:
- Visible when no component is selected
- Hidden when a component is already selected and saved
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx (1)
84-87: Consider adding test coverage for component selection.While the current test covers title editing, it should also verify component selection for new columns. Consider adding a test case that covers both scenarios:
- Editing a column with an existing component (current test)
- Editing a new column that requires component selection
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14476 +/- ##
=======================================
Coverage 95.69% 95.69%
=======================================
Files 1888 1888
Lines 24556 24558 +2
Branches 2818 2819 +1
=======================================
+ Hits 23498 23500 +2
Misses 799 799
Partials 259 259 ☔ View full report in Codecov by Sentry. |
Description
This PR handles the issue of missing references to the copied component when creating and saving a new column in a subform table. The component selector is now hidden when reopening a column with a preselected/copied component, while still allowing the user to edit the column's content.
hide-component-selector.mp4
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Summary by CodeRabbit
Tests
EditColumnElementcomponent.Refactor
ColumnElementPropstoEditColumnElementProps.