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

Resize column via keyboard #3357

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Resize column via keyboard #3357

wants to merge 10 commits into from

Conversation

godon019
Copy link
Contributor

@godon019 godon019 commented Oct 7, 2023

Closes #3320

@godon019 godon019 self-assigned this Oct 7, 2023
@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.21%. Comparing base (fe98d87) to head (e14b606).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3357   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files          47       47           
  Lines        5087     5091    +4     
  Branches      727      727           
=======================================
+ Hits         4996     5000    +4     
  Misses         91       91           
Files Coverage Δ
src/DataGrid.tsx 99.34% <100.00%> (+<0.01%) ⬆️

src/DataGrid.tsx Outdated
@@ -568,6 +569,19 @@ function DataGrid<R, SR, K extends Key>(
}
}

if (column.resizable && isCtrlKeyHeldDown(event) && event.shiftKey) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need some insights here @amanmahajan7 @nstepien

  1. should we provide 'resizeViaKeyboard' flag? so that user can turn it on and off?
  2. what is the best way to provide handler via API? not sure if I can use onCellKeyDown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to go with using the onCellKeyDown API. but still not sure as this doesn't seem to be very intrinsic

Copy link
Contributor

@amanmahajan7 amanmahajan7 Mar 4, 2024

Choose a reason for hiding this comment

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

I think we can always allow keyboard resizing if the column can be resized. I don't think we need an api to disable it unless there is a use case. We allow keyboard navigation on the header cell and can allow resizing when the cell is focused.

  • Check if the cell is focused and column is resizable
  • Check if a particular key combination is pressed (Ctrl + arrow right/left?)

Questions:

  • Which key to use. Are there any grid examples we can find?
    - Do we need some sort of timer if the key combination is pressed and increment the width by 1px every few ms? Does not look like this is an issue. The example works great. May be we can decrease the step 🤔

if (mode === 'EDIT') return;

if (onCellKeyDown && isRowIdxWithinViewportBounds(rowIdx)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we need to check isRowIdxWithinViewportBounds before call onCellKeyDown ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think because row would be undefined otherwise. Same for summary row. We may have to add a type HEADER | ROW | SUMMARY

@godon019 godon019 marked this pull request as ready for review October 17, 2023 18:43
@kycutler
Copy link
Contributor

Hi folks, thanks for working on this! Please let me know if I can help at all.

One bit of feedback on the changes so far: ideally resizeColumn and getColumnWidth would be accessible from anywhere and not limited to specific handlers like onCellKeyDown so consumers have flexibility in how it is exposed (for example, column widths could be rendered within the grid and made editable via input elements). Perhaps the methods could be present on the CalculatedColumns themselves, or exposed via imperative handle on the grid?

Thanks for all the work on this fantastic project!

@amanmahajan7
Copy link
Contributor

Should we complete and merge this PR?

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.

[Accessibility] Make columns resizable via keyboard
3 participants