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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Table row/column extension UI #1172
feat: Table row/column extension UI #1172
Changes from 1 commit
ca75523
ec4f300
41fa26f
cfb0168
2647bfc
e439815
b9c01e4
6e3487e
195e6c8
8352718
12270e3
ade2067
0eb643b
6b74eb8
d750891
ae218b0
1a4b0d2
e79f99f
8fb22ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confused here by row / cols. when orientation is row, shouldn't we use event.clientY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it is kind of confusing, you could justify either the bottom or right placement as being the
row
orientation. Imo it makes most sense if the button on the right is forrow
, since it extends the rows in the table and the table handle for therow
orientation is on the left (so it's consistent withrow
being left/right andcol
being top/bottom).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a) it looks like we set something on the state to then trigger a useEffect, that's not a very clear pattern I think
b) can't we just use the onClick handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a choice here because we need the
mousemove
andmouseup
handlers to be attached to the document body, because the mouse cursor is not always above the extend button while dragging. This is also why we can't use a click event, as those only fire when the same element is hovered on both the mouse down and mouse up.