feat(grid): add mouse hover show align lines #3768
Conversation
WalkthroughAdds hover alignment guides to TinyGrid: new demo components and docs entry, theme variables and styles for vertical/horizontal dashed bars, body rendering and sizing of alignment bar elements, cell mouseenter/mouseleave logic to show/hide and position bars, and a conditional container class toggled by Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant G as Grid Container
participant C as Cell
participant E as useCellEvent
participant B as Body (align bars)
participant S as Element Store
U->>C: pointer enters cell
C->>E: mouseenter
E->>S: read main-body-alignXBar / alignYBar refs
E->>B: showAlignLines(cellRect, scrollOffsets)
Note right of B: set CSS vars (--align-left/top), display:block
E->>G: emit existing hover events
U->>C: pointer leaves cell
C->>E: mouseleave
E->>B: hideAlignLines() → display:none
rect rgb(245,250,255)
note over G: Active only if `mouseConfig.hover === true` (container has `tiny-grid__hover-align`)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
examples/sites/demos/pc/app/grid/mouse-keyboard/mouse-config-hover.vue (1)
10-18: Clarify duplicate column definitions.Lines 10-14 and 15-18 appear to define duplicate columns (same fields: name, area, address, introduction). While this may be intentional to demonstrate horizontal scrolling with alignment guides, it creates potential confusion:
- Two columns with
field="name"(lines 11 and 15) binding to the same data field- The composition-api variant (mouse-config-hover-composition-api.vue) doesn't include these duplicates
If the duplication is intentional for testing purposes, consider:
- Adding a comment explaining the duplication is for horizontal scroll testing
- Using different field names or column titles to distinguish them
- Ensuring both demo variants are consistent in their approach to showcasing the hover feature
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/sites/demos/pc/app/grid/mouse-keyboard/mouse-config-hover-composition-api.vue(1 hunks)examples/sites/demos/pc/app/grid/mouse-keyboard/mouse-config-hover.vue(1 hunks)examples/sites/demos/pc/app/grid/webdoc/grid-mouse-keyboard.js(1 hunks)packages/theme-saas/src/grid/table.less(1 hunks)packages/vue/src/grid/src/body/src/body.tsx(4 hunks)packages/vue/src/grid/src/composable/useCellEvent.ts(3 hunks)packages/vue/src/grid/src/table/src/table.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (8)
examples/sites/demos/pc/app/grid/webdoc/grid-mouse-keyboard.js (1)
56-64: LGTM!The demo metadata is properly structured with localized names, descriptions, and code file references for the new hover alignment feature.
packages/vue/src/grid/src/table/src/table.ts (1)
877-877: LGTM!The conditional class binding follows the established pattern for other mouse configuration options and correctly applies the hover alignment styles when
mouseConfig.hoveris enabled.packages/vue/src/grid/src/composable/useCellEvent.ts (1)
116-146: Verify border offset calculation.The alignment bar positioning logic looks correct, but line 144 subtracts 1 from
cell.offsetWidth:yBar.style.setProperty('--after-left-offset', cell.offsetWidth - 1 + 'px')This might be intentional to account for borders or prevent overlap, but please verify this produces the correct visual alignment. If the cell has a 1px border, this adjustment may be needed; otherwise, it could cause a slight misalignment.
Run the demo and verify the vertical alignment line (right edge) aligns perfectly with the cell boundary when hovering over cells with and without borders.
packages/vue/src/grid/src/body/src/body.tsx (3)
726-727: LGTM!The alignment bar refs are properly declared and integrated into the component lifecycle.
751-760: LGTM!The refs are correctly added to the watch dependencies and stored in
elemStorefor access by the event handlers inuseCellEvent.ts.
870-881: LGTM!The alignment bars are conditionally rendered when hover is enabled, with proper dimensions tied to the scroll container. The structure follows Vue/TSX best practices.
packages/theme-saas/src/grid/table.less (2)
730-737: LGTM!The hover-align class correctly applies hover backgrounds to body columns and rows when the alignment feature is active.
740-805: LGTM!The alignment bar styles are well-structured:
- Hidden by default (
display: none)- Positioned absolutely with appropriate z-index
- Dashed borders implemented via pseudo-elements for clean rendering
- CSS custom properties enable dynamic positioning from JavaScript
- Consistent styling for both vertical (y-bar) and horizontal (x-bar) guides
examples/sites/demos/pc/app/grid/mouse-keyboard/mouse-config-hover-composition-api.vue
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/sites/demos/apis/pager.js (1)
113-114: Consider adding version info to English description.The Chinese description (line 113) mentions that
simplestwas added in version 3.19.0, but the English description (line 114) doesn't include this helpful context.Apply this diff to improve consistency:
- 'en-US': 'Set the display mode of pagination components' + 'en-US': 'Set the display mode of pagination components; simplest mode added in 3.19.0'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/sites/demos/apis/pager.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (1)
examples/sites/demos/apis/pager.js (1)
101-101: Separate pager docs changes from grid feature PR
Thepager.jsupdates (addingsimplest-pager) are unrelated to the grid hover alignment feature. Move these documentation edits into a dedicated PR for the newsimplest-pagerfeature. Also ensure:
- The en-US description lists
simplest-pager.- Comma spacing is consistent (e.g.
pager、 simplest-pager).
| '组件布局，子组件名用英文逗号分隔，子组件类型有 sizes、 prev、 pager、simplest-pager、 next、 jumper、 current、 total、 slot', | ||
| 'en-US': | ||
| 'Component layout, with sub component names separated by commas; The optional values for this attribute are sizes, preview, pager, next, jumper, current, total, slot.' |
There was a problem hiding this comment.
Fix formatting inconsistency and add English translation.
Two issues with the simplest-pager addition:
- Formatting inconsistency: The Chinese description has inconsistent spacing. Most items have a space after the comma (e.g.,
sizes、 prev), butpager、simplest-pagerlacks the space beforesimplest-pager. - Missing English translation: Line 103 (English description) doesn't mention
simplest-pager, creating incomplete documentation.
Apply this diff to fix both issues:
- '组件布局,子组件名用英文逗号分隔,子组件类型有 sizes、 prev、 pager、simplest-pager、 next、 jumper、 current、 total、 slot',
+ '组件布局,子组件名用英文逗号分隔,子组件类型有 sizes、 prev、 pager、 simplest-pager、 next、 jumper、 current、 total、 slot',
'en-US':
- 'Component layout, with sub component names separated by commas; The optional values for this attribute are sizes, preview, pager, next, jumper, current, total, slot.'
+ 'Component layout, with sub component names separated by commas; The optional values for this attribute are sizes, prev, pager, simplest-pager, next, jumper, current, total, slot.'📝 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.
| '组件布局，子组件名用英文逗号分隔，子组件类型有 sizes、 prev、 pager、simplest-pager、 next、 jumper、 current、 total、 slot', | |
| 'en-US': | |
| 'Component layout, with sub component names separated by commas; The optional values for this attribute are sizes, preview, pager, next, jumper, current, total, slot.' | |
| '组件布局,子组件名用英文逗号分隔,子组件类型有 sizes、 prev、 pager、 simplest-pager、 next、 jumper、 current、 total、 slot', | |
| 'en-US': | |
| 'Component layout, with sub component names separated by commas; The optional values for this attribute are sizes, prev, pager, simplest-pager, next, jumper, current, total, slot.' |
🤖 Prompt for AI Agents
In examples/sites/demos/apis/pager.js around lines 101 to 103, the Chinese list
has inconsistent spacing before "simplest-pager" and the English string omits
"simplest-pager"; update the Chinese string to consistently include a space
after each delimiter (e.g., change "pager、simplest-pager" to "pager、
simplest-pager") and update the English string to include "simplest-pager" in
the list of optional values (ensuring wording and punctuation match the
surrounding entries).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/theme/src/grid/table.less (3)
651-655: Consider adding row hover styles for completeness.The
__hover-alignblock only styles column hover. Since alignment guides typically help users align both rows and columns, consider adding row hover styles as well for a complete alignment experience.Apply this diff to add row hover styles:
&__hover-align { .@{grid-prefix-cls}-body__column:hover { background-color: var(--tv-Grid-bg-color-selected); } + .@{grid-prefix-cls}-body__row:hover { + background-color: var(--tv-Grid-bg-color-selected); + } }
687-713: Consider extracting common alignment bar styles.The horizontal and vertical alignment bars share significant styling (display, z-index, position, dashed border style). Consider extracting common styles to reduce duplication and improve maintainability.
Example refactor using a mixin or shared class:
// Common alignment bar styles .AlignmentBarBase() { display: none; z-index: 9; position: absolute; top: 0; left: 0; &:before, &:after { content: ''; position: absolute; display: block; border-style: dashed; border-color: var(--tv-Grid-align-line-border-color); } } & &-body__hover-align-y-bar { .AlignmentBarBase(); --after-left-offset: 0; &:before, &:after { height: 100%; border-left-width: 1px; } // ... rest of y-bar specific styles } & &-body__hover-align-x-bar { .AlignmentBarBase(); --after-top-offset: 0; height: 0; &:before, &:after { width: 100%; border-top-width: 1px; } // ... rest of x-bar specific styles }
658-685: Verify and adjust z-index for alignment bar.
- It’s set to z-index: 9, which is below the scrollbar (20) and resizer bar (21); raise it to ≥22 if the guides must render above those.
- Add a brief comment explaining the purpose of
--after-left-offsetfor future maintainability.examples/sites/demos/apis/grid.js (1)
3450-3452: Consider enhancing the property description.The
hoverproperty documentation could be more descriptive to help developers understand what the alignment guide does and when to use it.Suggested improvement:
interface IMouseConfig { // 是否开启左键选中单元格功能(只对 editConfig.mode=cell 有效),默认为 false selected: boolean - // 悬浮是否显示对齐辅助线(3.27.0新增) + // 是否在单元格悬浮时显示行列对齐辅助线,帮助用户对齐查看数据(3.27.0新增),默认为 false hover: boolean }examples/sites/demos/pc/app/grid/mouse-keyboard/mouse-config-hover.vue (1)
10-18: Clarify purpose of duplicate columns in hover demo
The template defines each data column twice—add a comment explaining this is intentional (to demo horizontal scrolling alignment) or remove the duplicate set if it’s unintentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/sites/demos/apis/grid.js(1 hunks)examples/sites/demos/pc/app/grid/mouse-keyboard/mouse-config-hover-composition-api.vue(1 hunks)examples/sites/demos/pc/app/grid/mouse-keyboard/mouse-config-hover.vue(1 hunks)packages/theme-saas/src/grid/table.less(2 hunks)packages/theme/src/grid/table.less(1 hunks)packages/theme/src/grid/vars.less(1 hunks)packages/vue/src/grid/src/body/src/body.tsx(4 hunks)packages/vue/src/grid/src/composable/useCellEvent.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/vue/src/grid/src/composable/useCellEvent.ts
- examples/sites/demos/pc/app/grid/mouse-keyboard/mouse-config-hover-composition-api.vue
- packages/vue/src/grid/src/body/src/body.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (4)
packages/theme/src/grid/vars.less (1)
247-248: LGTM!The new CSS variable for alignment line border color follows the established naming convention and uses an appropriate fallback value. The use of
var(--tv-color-bg-active-control, #1476ff)ensures consistency with the theme's active control color.examples/sites/demos/pc/app/grid/mouse-keyboard/mouse-config-hover.vue (1)
1-20: LGTM! Demo effectively showcases the hover alignment feature.The demo configuration is appropriate:
- Enables hover alignment with
mouse-config: { hover: true }- Includes editable cells to demonstrate alignment during editing
- Sets keyboard config for enhanced interaction
- Uses appropriate grid dimensions and column widths
packages/theme-saas/src/grid/table.less (2)
771-803: LGTM! Horizontal alignment bar implementation is consistent.The horizontal alignment bar follows the same structure as the vertical bar and mirrors the base theme implementation appropriately (accounting for SAAS theme's @apply syntax).
738-769: Alignment bar color matches base theme (#1476ff)
border-color-border-focusin the SAAS theme resolves to the brand token #1476ff, matching the fallback for--tv-Grid-align-line-border-color. No inconsistencies detected.
| &__hover-align { | ||
| .@{grid-prefix-cls}-body__column:hover { | ||
| @apply bg-color-fill-6; | ||
| } | ||
| .@{grid-prefix-cls}-body__row:hover { | ||
| @apply bg-color-fill-6; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify consistency with base theme implementation.
The SAAS theme's __hover-align block includes both column and row hover styles (lines 729-734), while the base theme (packages/theme/src/grid/table.less) only includes column hover styles. This inconsistency should be intentional or corrected.
Additionally, both themes use different background color approaches:
- Base theme:
var(--tv-Grid-bg-color-selected) - SAAS theme:
@apply bg-color-fill-6
Run the following script to compare the implementations:
🏁 Script executed:
#!/bin/bash
# Compare hover-align implementations between base and SAAS themes
echo "=== Base theme hover-align block ==="
rg -A5 "&__hover-align" packages/theme/src/grid/table.less
echo -e "\n=== SAAS theme hover-align block ==="
rg -A5 "&__hover-align" packages/theme-saas/src/grid/table.lessLength of output: 379
Verify and align hover styles and background color in SAAS theme
- The SAAS theme’s &__hover-align (packages/theme-saas/src/grid/table.less) includes both column and row hover, whereas the base theme only targets columns. Confirm if the row hover is intentional; otherwise remove it or add it to the base theme.
- Unify background-color usage: base uses
var(--tv-Grid-bg-color-selected), SAAS uses@apply bg-color-fill-6. Choose one approach for consistency across themes.
🤖 Prompt for AI Agents
In packages/theme-saas/src/grid/table.less around lines 728 to 735, the SAAS
theme adds hover rules for both columns and rows
(.@{grid-prefix-cls}-body__column:hover and .@{grid-prefix-cls}-body__row:hover)
while the base theme only targets columns, and SAAS uses @apply bg-color-fill-6
instead of the base CSS variable; decide whether row hover is intentional—if
not, remove the .@{grid-prefix-cls}-body__row:hover rule from this file (or if
it should be global, add the equivalent rule to the base theme file instead);
then unify background color usage by replacing @apply bg-color-fill-6 with the
agreed approach (prefer using var(--tv-Grid-bg-color-selected) for cross-theme
consistency) and ensure that the CSS variable is defined and documented in the
SAAS theme variables file.
gimmyhehe
changed the title
2 participants
PR
新增单元格悬浮对齐辅助线
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Documentation