Skip to content
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

Move drag handle outside the cell #3334

Merged
merged 12 commits into from
Sep 6, 2023
Merged

Move drag handle outside the cell #3334

merged 12 commits into from
Sep 6, 2023

Conversation

amanmahajan7
Copy link
Contributor

@amanmahajan7 amanmahajan7 commented Sep 5, 2023

Pros:

  • Cleans up row and cell props

Cons

  • Needs special handling for col spans

@amanmahajan7 amanmahajan7 self-assigned this Sep 5, 2023
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #3334 (e06ccf5) into main (6876768) will decrease coverage by 0.30%.
The diff coverage is 69.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3334      +/-   ##
==========================================
- Coverage   83.38%   83.09%   -0.30%     
==========================================
  Files          47       47              
  Lines        4910     4932      +22     
  Branches      752      757       +5     
==========================================
+ Hits         4094     4098       +4     
- Misses        816      834      +18     
Files Changed Coverage Δ
src/Cell.tsx 91.52% <ø> (-0.15%) ⬇️
src/Row.tsx 96.49% <ø> (-0.07%) ⬇️
src/DragHandle.tsx 72.59% <60.60%> (-10.60%) ⬇️
src/DataGrid.tsx 88.13% <100.00%> (+0.06%) ⬆️
src/TreeDataGrid.tsx 78.99% <100.00%> (-0.10%) ⬇️

@amanmahajan7 amanmahajan7 marked this pull request as ready for review September 5, 2023 18:47
gridColumnStart: column.idx + 1,
gridRowStart,
insetInlineStart:
column.frozen && typeof column.width === 'number'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this can cause any issues

src/DragHandle.tsx Outdated Show resolved Hide resolved
src/DataGrid.tsx Outdated Show resolved Hide resolved
function getDragHandle(rowIdx: number) {
if (selectedPosition.rowIdx !== rowIdx || selectedPosition.mode === 'EDIT' || onFill == null) {
function renderDragHandle() {
if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check whether the selected cell is not in a summary row?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isCellWithinViewportBounds would return false for header and summary rows. There is another function isCellWithinSelectionBounds that can be used to check header and summary rows as well

src/DragHandle.tsx Outdated Show resolved Hide resolved
amanmahajan7 and others added 3 commits September 6, 2023 08:31
Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>
Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>
@amanmahajan7 amanmahajan7 merged commit 4af02f1 into main Sep 6, 2023
3 checks passed
@amanmahajan7 amanmahajan7 deleted the am-drag-handle branch September 6, 2023 14:24
adityatoshniwal pushed a commit to pgadmin-org/react-data-grid that referenced this pull request Jul 31, 2024
* Move drag handle outside the cell

* alternate approach

* revert

* Remove margin

* Fix drag handle position on frozen column

* Fix typo

* Fix colSpan

* Simplify styles

* Update src/DragHandle.tsx

Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>

* Update src/DataGrid.tsx

Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>

* Fix if condition

---------

Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants