Skip to content

Conversation

@snowystinger
Copy link
Member

@snowystinger snowystinger commented Jun 22, 2022

Closes #3027
Closes #3026

The Resizer should only be visible during hover or after selecting resize from our Column Header menu.
You should be able to resize the column by hovering over it and clicking and dragging the Resizer.
You should be able to resize the column by keyboard navigating to the Column Header, opening the menu, and selecting resize. This should move focus to the resizer and then you can resize with the left and right arrow keys.
You should be able to resize the column by touching the Column Header with your finger to open the menu and selecting resize. Then touching and dragging the resizer left or right with your finger.
You should be able to cancel out of keyboard resizing using the Escape key or Enter key, which will return focus to the Column Header. Or, you can hit Tab/Shift+Tab to take you out of resizing mode and currently places you on the next cell.
Clicking elsewhere on the page or table should also take you out of keyboard resizing and edit mode.

In LTR, (this statement is flipped in RTL):
This PR also addresses our EW-resize cursor display. At min, it should only be an east resizer, at max it should only be a west resizer.

Notes: A partial implementation of edit mode was needed for this so that we could prevent grid navigation while resizing with the keyboard.
FocusScope changes were needed: contain can now be turned false in the middle of its life cycle, this was needed so that we could turn off containment in Menu's that are fading out. restoring focus should check if focus was lost before actually restoring.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@adobe-bot
Copy link

@adobe-bot
Copy link

columns: GridNode<T>[]
}

export function useTableColumnResizeState<T>(props: ColumnResizeStateProps, state: ColumnState<T>): ColumnResizeState<T> {
Copy link
Member Author

Choose a reason for hiding this comment

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

should the signature actually be


interface Column {
  key: Key,
  defaultWidth: number,
  width: number,
  minWidth: number,
  maxWidth: number
}

interface ColumnState {
  columns: Column[]
}

export function useTableColumnResizeState<T>(props: ColumnResizeStateProps, state: ColumnState): ColumnResizeState<T> {

Copy link
Member

Choose a reason for hiding this comment

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

Would this require extra changes to the TableCollection type definition? Wondering because we pass state.collection to this hook which has columns: GridNode<T>[]. Otherwise I guess we could construct an array of column data to match what you have above and pass that to useTableColumnResizeState, but what is the particular reason for the having the above interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

trying to decouple from our collections so someone could potentially use this with any table

Copy link
Member Author

Choose a reason for hiding this comment

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

discussed with devon, i'll handle changing the interface in another PR

@adobe-bot
Copy link

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Out of scope of this PR perhaps, but I found the resizer not lining up with the divider to be kinda weird:
image

columns: GridNode<T>[]
}

export function useTableColumnResizeState<T>(props: ColumnResizeStateProps, state: ColumnState<T>): ColumnResizeState<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Would this require extra changes to the TableCollection type definition? Wondering because we pass state.collection to this hook which has columns: GridNode<T>[]. Otherwise I guess we could construct an array of column data to match what you have above and pass that to useTableColumnResizeState, but what is the particular reason for the having the above interface?

@adobe-bot
Copy link

@adobe-bot
Copy link

@snowystinger snowystinger marked this pull request as ready for review June 30, 2022 01:12
@adobe-bot
Copy link

@adobe-bot
Copy link

@adobe-bot
Copy link

@adobe-bot
Copy link

let {pressProps} = usePress({
// Disabled for allowsResizing because if resizing is allowed, a menu trigger is added to the column header.
isDisabled: !allowsSorting || isSelectionCellDisabled || allowsResizing,
isDisabled: (!(allowsSorting || allowsResizing)) || isSelectionCellDisabled,
Copy link
Member Author

Choose a reason for hiding this comment

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

have some resizing logic in here i'd like to move out if anyone has any ideas, may do in follow up

@adobe-bot
Copy link

@adobe-bot
Copy link

reidbarber
reidbarber previously approved these changes Jul 1, 2022
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM, tested on Chrome, Safari, FF, and iOS Safari

@devongovett
Copy link
Member

Looks good! A few minor things I discovered while testing, to address in followups.

  1. After dragging a resizer with a mouse, it remains blue even when removing hover and coming back. I guess because it's focused. Should we even focus it with a mouse? If so, then where should we send focus afterward? Maybe the blue should only be the keyboard focus style unless it is actively being dragged?
  2. Resizer doesn't line up with divider between cells (see screenshot below).
  3. The e-resize/w-resize cursors don't match the col-resize cursor. One renders as an arrow, the other has a vertical line too. I like the col-resize cursor better - wonder if there is a way to make it only have one directional arrow without changing the style. Might require custom cursors, which I guess Spectrum does define...
  4. When hovering over the column headers, only the resizer for that column shows up, but I think it might make sense to show all column resizers?
  5. Probably should have some indicator (maybe a chevron) on hover to show that there is a menu.
  6. Resizer probably doesn't have enough contrast when not focused. It's very difficult to see.

Screen Shot 2022-07-01 at 3 30 30 PM

@adobe-bot
Copy link

devongovett
devongovett previously approved these changes Jul 1, 2022
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Approving pending one update

@adobe-bot
Copy link

@devongovett devongovett merged commit 3732448 into main Jul 1, 2022
@devongovett devongovett deleted the resizer-keyboard branch July 1, 2022 23:49
@matthewdeutsch matthewdeutsch linked an issue Aug 10, 2022 that may be closed by this pull request
@snowystinger snowystinger mentioned this pull request Aug 19, 2022
9 tasks
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.

Grid navigation edit mode Column resizing followup - task: Fix focus marshalling to resizer Column resizing followup - task: Support RTL

6 participants