feat(core): merged before/after drop indicator #13612
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
darkskygit
changed the title
WalkthroughImplements a new geometry calculation for the drop indicator in _getDropResult for non-insert placements. When dropping before/after, it computes a collapsed visual indicator spanning adjacent blocks using derived line height and rect bounds; existing behavior for “in” and edge placements remains, with the final rect using the new indicator when applicable. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant D as DragEventWatcher
participant G as Geometry Calculator
participant UI as Drop Indicator
U->>D: Drag block over target
D->>G: _getDropResult(event, placement)
alt placement == "in" or edge-based
G-->>D: Compute existing rect (unchanged)
else placement == "before"/"after"
G->>G: Find parent and adjacent block
G->>G: Measure rects, derive lineHeight
G-->>D: Compute collapsed indicator (left, width, top)
end
D->>UI: Render indicator with final Rect
note right of UI: Uses new indicator geometry when available
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #13612 +/- ##
=======================================
Coverage 56.75% 56.76%
=======================================
Files 2755 2755
Lines 136963 136983 +20
Branches 20956 20965 +9
=======================================
+ Hits 77730 77755 +25
- Misses 57015 57020 +5
+ Partials 2218 2208 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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)
blocksuite/affine/widgets/drag-handle/src/watchers/drag-event-watcher.ts (1)
287-294: Guard against index === -1 to avoid incorrect adjacent pickIf
findIndexreturns -1,placement === 'after'will readsiblings[0], causing a wrong span. Add an index check before accessing siblings.- const siblings = parent.children; - const index = siblings.findIndex(child => child.id === dropModel.id); - const adjacentModel = - placement === 'before' ? siblings[index - 1] : siblings[index + 1]; + const siblings = parent.children; + const index = siblings.findIndex(child => child.id === dropModel.id); + const adjacentModel = + index >= 0 + ? (placement === 'before' ? siblings[index - 1] : siblings[index + 1]) + : undefined;Please verify if
dropModelcan ever be temporarily out ofparent.childrenduring drag; if so, we might also early-return to the simple (non-merged) indicator whenindex < 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
blocksuite/affine/widgets/drag-handle/src/watchers/drag-event-watcher.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
blocksuite/affine/widgets/drag-handle/src/watchers/drag-event-watcher.ts (2)
blocksuite/affine/shared/src/utils/dom/point-to-block.ts (1)
getRectByBlockComponent(266-269)blocksuite/affine/widgets/page-dragging-area/src/utils.ts (1)
Rect(9-14)
⏰ 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). (48)
- GitHub Check: E2E Test (10)
- GitHub Check: E2E Test (3)
- GitHub Check: E2E Test (5)
- GitHub Check: E2E Test (4)
- GitHub Check: E2E Test (9)
- GitHub Check: E2E Test (8)
- GitHub Check: E2E Test (7)
- GitHub Check: E2E Test (6)
- GitHub Check: E2E BlockSuite Cross Browser Test (2, firefox)
- GitHub Check: E2E Test (2)
- GitHub Check: E2E Test (1)
- GitHub Check: E2E BlockSuite Cross Browser Test (2, chromium)
- GitHub Check: E2E BlockSuite Cross Browser Test (2, webkit)
- GitHub Check: E2E BlockSuite Cross Browser Test (1, chromium)
- GitHub Check: E2E BlockSuite Cross Browser Test (1, firefox)
- GitHub Check: E2E BlockSuite Cross Browser Test (1, webkit)
- GitHub Check: y-octo binding test on x86_64-pc-windows-msvc
- GitHub Check: Build @affine/electron renderer
- GitHub Check: y-octo binding test on x86_64-apple-darwin
- GitHub Check: Build AFFiNE native (x86_64-apple-darwin)
- GitHub Check: y-octo binding test on aarch64-pc-windows-msvc
- GitHub Check: Build AFFiNE native (x86_64-pc-windows-msvc)
- GitHub Check: Build AFFiNE native (aarch64-pc-windows-msvc)
- GitHub Check: Build Server native
- GitHub Check: fuzzing
- GitHub Check: Run native tests
- GitHub Check: loom thread test
- GitHub Check: E2E Mobile Test (3)
- GitHub Check: E2E Mobile Test (5)
- GitHub Check: E2E BlockSuite Test (5)
- GitHub Check: E2E Mobile Test (2)
- GitHub Check: E2E Mobile Test (4)
- GitHub Check: E2E BlockSuite Test (6)
- GitHub Check: E2E Mobile Test (1)
- GitHub Check: E2E BlockSuite Test (8)
- GitHub Check: E2E BlockSuite Test (7)
- GitHub Check: E2E BlockSuite Test (10)
- GitHub Check: E2E BlockSuite Test (9)
- GitHub Check: E2E BlockSuite Test (4)
- GitHub Check: E2E BlockSuite Test (1)
- GitHub Check: E2E BlockSuite Test (3)
- GitHub Check: E2E BlockSuite Test (2)
- GitHub Check: Lint
- GitHub Check: Analyze (typescript, blocksuite)
- GitHub Check: Analyze (javascript, blocksuite)
- GitHub Check: Analyze (typescript, affine)
- GitHub Check: Analyze (javascript, affine)
- GitHub Check: Typecheck
Comment on lines
+298
to
+314
| if (adjacentRect) { | ||
| // Collapse before/after visuals onto the same gap by spanning both blocks. | ||
| const upperRect = placement === 'before' ? adjacentRect : domRect; | ||
| const lowerRect = placement === 'before' ? domRect : adjacentRect; | ||
|
|
||
| const left = Math.min(upperRect.left, lowerRect.left); | ||
| const right = Math.max( | ||
| upperRect.left + upperRect.width, | ||
| lowerRect.left + lowerRect.width | ||
| ); | ||
|
|
||
| indicatorLeft = left; | ||
| indicatorWidth = right - left; | ||
| indicatorTop = lowerRect.top - lineHeight * 2; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Merged indicator vertical offset likely off by one thickness
Using indicatorTop = lowerRect.top - lineHeight * 2 places the line one extra thickness above the gap. Aligning it to the boundary (just above the lower block) should be - lineHeight. If you want the line centered between blocks, compute the mid-gap.
Apply one of these:
- Boundary-aligned (matches prior before/after behavior):
- indicatorTop = lowerRect.top - lineHeight * 2;
+ indicatorTop = lowerRect.top - lineHeight;- Centered in the gap (if desired by design):
- indicatorTop = lowerRect.top - lineHeight * 2;
+ const gapTop = upperRect.top + upperRect.height;
+ const gapBottom = lowerRect.top;
+ const mid = (gapTop + gapBottom) / 2;
+ indicatorTop = mid - lineHeight / 2;📝 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.
Suggested change
| if (adjacentRect) { | |
| // Collapse before/after visuals onto the same gap by spanning both blocks. | |
| const upperRect = placement === 'before' ? adjacentRect : domRect; | |
| const lowerRect = placement === 'before' ? domRect : adjacentRect; | |
| const left = Math.min(upperRect.left, lowerRect.left); | |
| const right = Math.max( | |
| upperRect.left + upperRect.width, | |
| lowerRect.left + lowerRect.width | |
| ); | |
| indicatorLeft = left; | |
| indicatorWidth = right - left; | |
| indicatorTop = lowerRect.top - lineHeight * 2; | |
| } | |
| } | |
| } | |
| if (adjacentRect) { | |
| // Collapse before/after visuals onto the same gap by spanning both blocks. | |
| const upperRect = placement === 'before' ? adjacentRect : domRect; | |
| const lowerRect = placement === 'before' ? domRect : adjacentRect; | |
| const left = Math.min(upperRect.left, lowerRect.left); | |
| const right = Math.max( | |
| upperRect.left + upperRect.width, | |
| lowerRect.left + lowerRect.width | |
| ); | |
| indicatorLeft = left; | |
| indicatorWidth = right - left; | |
| indicatorTop = lowerRect.top - lineHeight; | |
| } |
Suggested change
| if (adjacentRect) { | |
| // Collapse before/after visuals onto the same gap by spanning both blocks. | |
| const upperRect = placement === 'before' ? adjacentRect : domRect; | |
| const lowerRect = placement === 'before' ? domRect : adjacentRect; | |
| const left = Math.min(upperRect.left, lowerRect.left); | |
| const right = Math.max( | |
| upperRect.left + upperRect.width, | |
| lowerRect.left + lowerRect.width | |
| ); | |
| indicatorLeft = left; | |
| indicatorWidth = right - left; | |
| indicatorTop = lowerRect.top - lineHeight * 2; | |
| } | |
| } | |
| } | |
| if (adjacentRect) { | |
| // Collapse before/after visuals onto the same gap by spanning both blocks. | |
| const upperRect = placement === 'before' ? adjacentRect : domRect; | |
| const lowerRect = placement === 'before' ? domRect : adjacentRect; | |
| const left = Math.min(upperRect.left, lowerRect.left); | |
| const right = Math.max( | |
| upperRect.left + upperRect.width, | |
| lowerRect.left + lowerRect.width | |
| ); | |
| indicatorLeft = left; | |
| indicatorWidth = right - left; | |
| const gapTop = upperRect.top + upperRect.height; | |
| const gapBottom = lowerRect.top; | |
| const mid = (gapTop + gapBottom) / 2; | |
| indicatorTop = mid - lineHeight / 2; | |
| } |
🤖 Prompt for AI Agents
In blocksuite/affine/widgets/drag-handle/src/watchers/drag-event-watcher.ts
around lines 298-314 the computed vertical position places the indicator one
extra thickness above the gap; replace the current indicatorTop assignment so it
either aligns to the boundary (indicatorTop = lowerRect.top - lineHeight) or is
centered in the gap by computing the mid-gap and offsetting half the line
thickness (indicatorTop = ((upperRect.top + upperRect.height) + lowerRect.top) /
2 - lineHeight / 2), choose the behavior required by design and remove the old -
lineHeight * 2 value.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi y'all, I noticed that when dragging and dropping a text block, two drop indicators appear between blocks: the “after” indicator of the upper block and the “before” indicator of the lower block. Although they look different visually, there is actually no functional difference when dropping a block.
I merged these two indicators into a single one that sits between blocks. I think this approach is more straightforward and less likely to confuse users. I'd love to know if you guys think this makes sense.
Thanks!
Summary by CodeRabbit