fix(nc-gui): expand row issue in canvas #10535
Conversation
📝 WalkthroughWalkthroughThe change updates the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as extractHoverMetaColRegions
participant R as Row State Checker
participant CR as Checkbox Renderer
U->>F: Hover over row
F->>R: Evaluate row state (hovered, checked, reordering, read-only)
R-->>F: Return row state details
F->>CR: If valid (hovered/checked, not reordering, not read-only), push checkbox region
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nc-gui/components/smartsheet/grid/canvas/index.vue (1)
474-538: Consider improving code maintainability with these refactors.
- Extract magic numbers as named constants for better readability:
+const ROW_META_PADDING = 4 +const CHECKBOX_PADDING = 6 +const ICON_PADDING = 8 +const REGION_WIDTH = 24 +const CHECKBOX_TOTAL_WIDTH = 30 function extractHoverMetaColRegions(row: Row) { const isAtMaxSelection = selectedRows.value.length >= MAX_SELECTED_ROWS const isCheckboxDisabled = (!row.rowMeta.selected && isAtMaxSelection) || vSelectedAllRecords.value || readOnly.value const isChecked = row.rowMeta?.selected || vSelectedAllRecords.value const isHover = hoverRow.value === row.rowMeta.rowIndex const regions = [] - let currentX = 4 + let currentX = ROW_META_PADDING let isCheckboxRendered = false if (isChecked || (selectedRows.value.length && isHover) || (isHover && !isRowReOrderEnabled.value && !readOnly.value)) { if (isChecked || isHover) { regions.push({ - x: currentX + 6, + x: currentX + CHECKBOX_PADDING, - width: 24, + width: REGION_WIDTH, action: isCheckboxDisabled ? 'none' : 'select', }) isCheckboxRendered = true - currentX += 30 + currentX += CHECKBOX_TOTAL_WIDTH } } // ... rest of the function
- Consider using a more functional approach to determine regions:
function getRegionConfig(row: Row, currentX: number): RegionConfig { if (shouldShowCheckbox(row)) { return { x: currentX + CHECKBOX_PADDING, width: REGION_WIDTH, action: isCheckboxDisabled(row) ? 'none' : 'select', offsetX: CHECKBOX_TOTAL_WIDTH } } // ... other region configs }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nc-gui/components/smartsheet/grid/canvas/index.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (2)
packages/nc-gui/components/smartsheet/grid/canvas/index.vue (2)
482-482: LGTM! The condition change improves UX by preventing conflicting interactions.The updated condition for showing the checkbox region now properly handles row reordering and read-only states, preventing the checkbox from appearing when:
- Row reordering is enabled, avoiding confusion between reorder and select actions
- The grid is in read-only mode, maintaining consistent read-only behavior
474-538:❓ Verification inconclusive
Verify test coverage for the updated checkbox visibility logic.
The changes to
extractHoverMetaColRegionsaffect when checkboxes are shown in the row meta column. Please ensure:
- Test cases cover the new condition, especially the interaction between row reordering and checkbox visibility
- Edge cases are tested (e.g., read-only mode with hover, reorder mode with hover)
Run this script to check for existing tests:
🏁 Script executed:
#!/bin/bash # Search for tests related to checkbox visibility in row meta column rg -t ts -t vue "extractHoverMetaColRegions|checkbox.*visibility|row.*reorder.*checkbox" test/Length of output: 124
Ensure Comprehensive Test Coverage for the Updated Checkbox Visibility Logic
The changes to
extractHoverMetaColRegionsintroduce new conditions that affect how checkboxes are rendered—especially during row reordering, in read-only mode, and when the maximum selection count is reached. Our initial search for tests referencing this function did not return any results (and encountered an issue with the file type filter for Vue files), so it's important to manually verify that:
Test cases exist (or are added) that cover:
- The interaction of row hover with row reordering.
- Scenarios in which checkboxes should be enabled or disabled, particularly when the maximum number of selected rows is reached or in read-only mode.
- Edge cases such as the interplay between
vSelectedAllRecords, hover state, and the reorder mode.Manual verification is performed for any functionality that isn't already covered by automated tests.
|
Uffizzi Preview |
Change Summary
Provide summary of changes with issue number if any.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of