TableView Column resizing #2883
Conversation
…rt percentage values for min/max
…of the columnWidth algorithm)
…r changes and were hidden bugs!)
Also refactor the way we recalculate columns to only re-calculate dynamic columns for performance and predictability for the onResize. Also refactor useColumnResizeWidthState out of useTableState.
utilizing useLayoutEffect because in the RSP version, this hook would end up depending on actual DOM measurements. That may not be true for other implementations of this hook so its possible requiring it isnt neceessary.
This reverts commit 1e2ee48.
| // Disabled for allowsResizing because if resizing is allowed, a menu trigger is added to the column header. | ||
| isDisabled: !allowsSorting || isSelectionCellDisabled || allowsResizing, |
There was a problem hiding this comment.
Perhaps a minor point:
Is adding a menu trigger to the column header when the column supports resizing an interaction pattern we'll be recommending for a non-Spectrum tables as well? If not, then I don't know if we wanna disable the press to sort interactions here, perhaps the pressProps from this hook should be overridden in the TableView instead.
Just thinking about a case where someone using our table hooks implements column resizing in a such a way that pressing the column headers should perform the sort action still and resizing via keyboard is handled in a different way (keyboard shortcut maybe? I'll need to dig to see if any other tables support resizing columns via keyboard).
| role="separator" | ||
| aria-orientation="vertical" | ||
| aria-label="Resize column" | ||
| aria-labelledby={item.key} |
There was a problem hiding this comment.
This won't actually point to a id that exists in the DOM. Maybe you could use getColumnHeaderId?
As an aside I noticed that the column headers also have lost their "columnheader" role in favor of the "button" role provided by MenuTrigger, note to self to double check that the rest of the attributes are still valid (it seems ok so far?)
| .add( | ||
| 'allowsResizing, uncontrolled, sortable columns', | ||
| () => <AsyncLoadingExample isResizable />, | ||
| {chromatic: {disable: true}} | ||
| ) |
There was a problem hiding this comment.
I noticed considerable slowdown when sorting a table with resizable columns vs non-resizable columns, even when dealing with a static collection. I'll need to dig into exactly why this is, but noting it for now
There was a problem hiding this comment.
After our virtualizer changes we noticed that the speed of sorting increased significantly. We're hoping those changes fixed the problem but this is something we also care about deeply and so it's the first thing we're planning on testing once we get it in our codebase.
There was a problem hiding this comment.
Sounds good, I'll give it a quick once over but def not blocker for merge
| @@ -10,6 +10,8 @@ | |||
| * governing permissions and limitations under the License. | |||
| */ | |||
|
|
|||
| export * from './useTableColumnResizeState'; | |||
| export * from './utils'; | |||
There was a problem hiding this comment.
Should these utils be exported? Are we expecting them to be used by other users? From what I can see they are just used by useTableColumnResizeState and the stately tests so perhaps we shouldn't export them
There was a problem hiding this comment.
We do use getContentWidth in the useTableColumnResizeState.test.ts file like you said, so do you recommend we remove this export and then import in our test file like so?
import {getContentWidth} from '../src/utils';
import {renderHook} from '@testing-library/react-hooks';
import {useTableColumnResizeState} from '../';
There was a problem hiding this comment.
Right, no need to export anything that users/other packages won't use. We can always export them in the future if the need arises but for now we should exclude it IMO
Co-authored-by: Daniel Lu <[email protected]>
Closes #2555
✅ Pull Request Checklist:
📝 Test Instructions:
TODOs as followup potentially
🧢 Your Project:
Adobe Analytics (Analysis Workspace)