fix(draw): adjust table cell edge resize - #1151
Conversation
Deploying plait with
|
| Latest commit: |
1dd0ec5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a06486e8.plait.pages.dev |
| Branch Preview URL: | https://codex-table-resize-cell-edge.plait.pages.dev |
Deploying plait-docs with
|
| Latest commit: |
1dd0ec5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3d280576.plait-docs.pages.dev |
| Branch Preview URL: | https://codex-table-resize-cell-edge.plait-docs.pages.dev |
281b6a4 to
6871efa
Compare
|
@nightt5879 I think this PR is ready for review, help me, please! |
nightt5879
left a comment
There was a problem hiding this comment.
A few edge cases I ran into while testing this.
|
I am still working on it, because I need to check the new resolution. |
d682b87 to
2ad1b16
Compare
|
Follow-up: I revisited the implementation and narrowed it back to the original cell-edge based approach. The current fix keeps |
c7d6edd to
4b9b4a1
Compare
|
Hi @nightt5879 The current implementation intentionally stays simple:
The persisted Help me review again, please |
|
Thanks for the update. I reviewed the current head again. I’m aligned with keeping this PR focused, and I have one merge-sequencing suggestion, one non-blocking finding, and one interaction question.
Overall, I do not consider points 2 or 3 blockers. For point 1, my main concern is making the merge dependency and follow-up timing explicit rather than expanding the scope of this PR unexpectedly. |
|
Thanks for reviewing again. For point 1, the follow-up PR is ready here: #1157. It normalizes the persisted For point 2, I agree this is a pre-existing resize issue. I will address it in a separate follow-up PR with focused coverage. For point 3, I agree the grid-intersection priority was not considered at that level of detail originally. The current interaction is acceptable for now, so I would prefer not to add more complex axis-priority handling in this PR. |
|
Thanks for the good eye on this resize-origin case. I fixed it by making the cell-resize path apply the computed target state on every resize frame, including when the cumulative pointer offset returns to zero. This restores the original column width and table points instead of leaving the last non-zero resize result in place. I also added a focused unit test covering: expand a column from 100 to 120, then move the pointer back to the pointer-down position, and verify the width/points return to the original state. Validation:
Could you please review again when you have a chance? |
2275724 to
e40bb1b
Compare
|
Good catch on the test shape. The previous unit test was not close enough to the real interaction because it called the helper twice against the same unchanged table, while the actual resize flow mutates I updated the fix and the test accordingly:
Validation:
|
e40bb1b to
1d5c59e
Compare
|
Correction: I reverted the unit-test-driven change from the previous update. After re-checking it, adding a resize-origin snapshot in production code just to make that unit test meaningful was not appropriate for this PR. The branch now keeps the fix minimal and does not add that unit test or the extra helper/snapshot logic. Current scope:
Validation:
|
|
I rechecked the latest commit. Removing the Since this was non-blocking, I’m fine with reverting the latest commit and handling it separately; otherwise, the fix needs an immutable pointer-down baseline (or equivalent) and a two-frame test. Also, although the PR is currently conflict-free, the head is still based before the latest |
1dd0ec5 to
51a2a3e
Compare
|
Final update for this review round. I rechecked the resize-origin issue with the board update model in mind. The correct reasoning is:
So the issue is real, but it is pre-existing and not part of the main interaction scope of this PR. I have left this PR unchanged and moved that focused fix into a separate follow-up PR: That follow-up is based on this PR branch so its diff only contains the resize-origin fix. After this PR lands, it can be retargeted or rebased onto This PR is now still focused on the table cell edge resize interaction, and it is already based on the current |
nightt5879
left a comment
There was a problem hiding this comment.
LGTM. Thanks for clarifying the immer-backed update model. I agree that the resize-origin issue is pre-existing and better handled separately in #1159, while keeping this PR focused.
Summary
This PR reworks the table resize interaction from the older #931 idea against the current
developbranch.The important behavior change is not just a code cleanup. It makes table resize intent explicit:
Interaction Intent Changes
1. Outer table edges now mean row/column resize, not whole-table resize
Before this change, when a user selected a table and moved the pointer to the right edge of the table, that position could match both:
The old hit-test path checked the table handle first, so the interaction was often interpreted as whole-table resize.
That meant the user appeared to be dragging the last column edge, but the system treated it as resizing the whole table.
After this change, cell resize handles are checked first. If the pointer hits the right edge of the last cell, the interaction is handled as a column resize.
2. Whole-table resize is now reserved for table corners
Before this change, both table side handles and table corner handles could trigger whole-table resize.
That made these two user intents compete with each other:
After this change, table-level resize only falls back to corner handles. Side edges are reserved for row/column resize.
This makes the interaction model mutually exclusive and easier to predict.
3. Start-side row/column resize is now explicit
The previous implementation could already move
points[0]through the table-level resize path. For example, when the interaction was handled as a whole-table resize,resizeSnapRef.activePointsplusnormalizeShapePointscould produce a new start point.The gap was in the cell row/column resize path. The old row/column update helpers behaved like end-side updates:
That is correct for right/bottom cell edges, but it does not clearly model the user intent when dragging a left/top cell edge.
When a user drags the left edge of the first column, the expected intent is not whole-table resize. The intent is:
So the row/column resize path itself needs to know whether the dragged edge is the start side or the end side.
This PR introduces a small
ResizeEdgeconcept:w / n->starte / s->endSo the row/column update now matches the dragged cell edge:
The improvement is therefore not first-time support for changing
points[0]; it is making start-side row/column resize explicit instead of relying on the table-level resize path.4. Merged cells resolve the resize target by dragged edge
Merged cells can cover multiple rows or columns.
For example, a cell with
colspan=3covers columns 1, 2, and 3.The expected interaction is:
The old logic mainly followed the cell starting
rowId/columnId, which made end-side resize ambiguous forrowspan/colspancells.This PR resolves the target row/column from the dragged edge:
So merged-cell resize now maps to the user visible drag target instead of always leaning on the cell starting row/column.
Implementation Notes
hitTestchecks cell side handles before table-level handles.ResizeEdgerepresents whether the dragged cell edge is the start or end side.getResizeTargetRowOrColumnIndexresolves the row/column affected by normal and merged cells.getCurrentRowOrColumnSizederives the current rendered row/column size from the table layout, avoiding fragile inference from cell rectangles.Verification
npm run build:drawFocused manual checks to run:
Unit tests can be added in a follow-up once the interaction behavior is confirmed.