Skip to content

Col resize hit area increase#3414

Merged
snowystinger merged 25 commits into
mainfrom
col-resize-hit-area-increase
Sep 19, 2022
Merged

Col resize hit area increase#3414
snowystinger merged 25 commits into
mainfrom
col-resize-hit-area-increase

Conversation

@snowystinger
Copy link
Copy Markdown
Member

@snowystinger snowystinger commented Aug 18, 2022

Closes

  • Increase hit area for resizers, investigate
    • Set zIndex for all column headers such that the earliest dom element in the row is the highest, thereby allowing us to overflow the resizer on top of the next header.
    • Open question, should I try to restrict it to only visible columns? My feeling is that we don't need to bother because the header row creates a stacking context, so these won't interfere with the rest of the table. Given that the visible window could expand, we technically need to allow for every possible zindex anyways, so it doesn't hurt to just line them all up. Biggest zindex possible is 2147483647, I think we won't need to worry about hitting that.
  • Resizer stays blue after resizing with mouse if you hover again
  • Firefox active state isn't blue, it should be blue like the other browsers
  • If the user resizes a column via mouse first and then tabs to the column header, keyboard navigation between column headers no longer works

✅ 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
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@snowystinger snowystinger marked this pull request as ready for review August 19, 2022 01:28
@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

reidbarber
reidbarber previously approved these changes Aug 23, 2022
Copy link
Copy Markdown
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, working as intended in Chrome/FF/Safari/iOS Safari.

@adobe-bot
Copy link
Copy Markdown

Copy link
Copy Markdown
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.

Confirmed that I can actually resize the columns with my Android phone now with the larger hit areas! Noticed one issue, feel free to add to a followup if you want

Comment thread packages/@react-aria/table/src/useTableColumnResize.ts Outdated
Comment thread packages/@react-aria/table/src/useTableColumnResize.ts
Comment thread packages/@react-spectrum/table/src/Nubbin.tsx
Comment thread packages/@react-spectrum/table/src/Nubbin.tsx
keysBefore.push(key);
key = tableState.collection.getKeyBefore(key);
} while (key != null);
let resizerPosition = keysBefore.reduce((acc, key) => acc + columnState.getColumnWidth(key), 0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you'll have to account for scroll position here, try going to https://reactspectrum.blob.core.windows.net/reactspectrum/64ad9faffb499da09831a4beeb35e78613d787a2/storybook/index.html?path=/story/tableview--allowsresizing-many-columns-and-rows&providerSwitcher-scale=large&providerSwitcher-toastPosition=bottom and scrolling a bit to the right and then trigger resizing via the column header dropdown. You'll see the indicator is offset from the actual resizer by the amount you scrolled.

A similar class of issue happens when you drag the column resizer of the first column past the width of the visible table, then trigger the resize mode for the same column via the dropdown. The column will scroll the resizer into view but the nubbin indicator will be in the position the resizer was in before scrolling

# Conflicts:
#	packages/@react-aria/table/intl/ar-AE.json
@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

LFDanLu
LFDanLu previously approved these changes Sep 16, 2022
Copy link
Copy Markdown
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.

Tested in Chrome/Safari/FF and Chrome Android, works pretty well. I did find one more edge case that I'll describe below, but I'm approving since it feels like exceeded the original scope of this PR so feel free to handle it in a followup alongside my other nitpicks

Repro:

  1. Navigate to https://reactspectrum.blob.core.windows.net/reactspectrum/86644b16aae2d50e58adf67798194f50dad07dcc/storybook/index.html?path=/story/tableview--allowsresizing-uncontrolled-dynamic-widths&providerSwitcher-toastPosition=bottom
  2. Hover over the "Type" column and drag its resizer to the right a little bit.
  3. Click on the "Focusable before" input and tab into the tableview. Sometimes focus moves to the first column header instead of to the "Type" column
  4. If focus is on the first column header, trigger keyboard resizing via the menu. Note that focus jumps from the resizer back to the "Type" column

Really weird and not 100% reproducible, must be something with the tracked focusedkey messing up.

Comment on lines +301 to +305
/* don't want the divider to add to the resizer's width since it's */
&:active,
&:focus-ring {
&::after {
background-color: var(--spectrum-global-color-blue-400);
background-color: unset;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

mega nit:
is this comment supposed to be here? Just trying to figure out how unsetting the background color is related to the resizer width here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've got an element that sits on top and isn't the button, when focused, I don't want it visible, it causes a tiny wiggle because the anti-aliasing is strong than it should be, so this looks better

Comment thread packages/@react-spectrum/table/src/Resizer.tsx Outdated
@adobe-bot
Copy link
Copy Markdown

LFDanLu
LFDanLu previously approved these changes Sep 16, 2022
@devongovett
Copy link
Copy Markdown
Member

Screen Shot 2022-09-16 at 5 45 21 PM (2)

2 things:

  1. The top border of the table is above the blue line.
  2. Should the blue line extend to the bottom of the table, rather than only to the bottom of the rows?

@adobe-bot
Copy link
Copy Markdown

Copy link
Copy Markdown
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.

One last nit, small visual defect in the resizer when it is at the edge of the table, looks like it follows the rounded corner of the table and thus has a small gap:
image

@adobe-bot
Copy link
Copy Markdown

Copy link
Copy Markdown
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.

LGTM

Copy link
Copy Markdown
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 Chrome/FF/Safari/iOS Safari.

@adobe-bot
Copy link
Copy Markdown

@snowystinger snowystinger merged commit 1728753 into main Sep 19, 2022
@snowystinger snowystinger deleted the col-resize-hit-area-increase branch September 19, 2022 22:32
@snowystinger snowystinger mentioned this pull request Sep 19, 2022
61 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.

6 participants