Tableview col resizing#3210
Conversation
snowystinger
changed the title
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Noticed the following issues that didn't exist on the previous build:
https://reactspectrum.blob.core.windows.net/reactspectrum/d736e9bb4c9b1b08cc43b40d535e7a181448b9d7/storybook/index.html?path=/story/tableview--allowsresizing-uncontrolled-static-widths&providerSwitcher-toastPosition=bottom
Resizing with any resize handle but the last one causes the last column to collapse.
similar issue exists in the following story:
https://reactspectrum.blob.core.windows.net/reactspectrum/d736e9bb4c9b1b08cc43b40d535e7a181448b9d7/storybook/index.html?path=/story/tableview--allowsresizing-hiding-columns&providerSwitcher-toastPosition=bottom
- Hide "Reach" and "Target OTP"
- Grab the right most resize handle and drag it as far to the left as you can
- Grab the next resize handle and drag it to the left. Note that it collapses the last column
| // setting the table width will recalculate column widths which we only want to do once the virtualizer is done initializing | ||
| if (state.virtualizer.contentSize.height > 0) { | ||
| setTableWidth(rect.width); | ||
| } | ||
| setTableWidth(rect.width); |
There was a problem hiding this comment.
Was this part of the spooky column code? Or an optimization?
There was a problem hiding this comment.
This was that bit of code that you and I looked at together when we were fixing the tests. I don't recall the exact reason, but we didn't need to wait for initialization. It's not related to the spooky column though. I'll try to find more of the reason for the change.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
devongovett
left a comment
devongovett
left a comment
There was a problem hiding this comment.
Had a couple questions, but approving to unblock future work.
| acc.push(options[key]); | ||
| } | ||
| return acc; | ||
| }, []); |
There was a problem hiding this comment.
Why is this written like this rather than just coding it as an array in the first place?
There was a problem hiding this comment.
we had some issues with null children in Menu's at one point, I might've been defensively coding against that memory
I'll update in my next PR
| manyItems.push({id: i, foo: 'Foo ' + i, bar: 'Bar ' + i, baz: 'Baz ' + i}); | ||
| } | ||
|
|
||
| describe('TableViewSizing', function () { |
There was a problem hiding this comment.
Effort to make test file smaller, it was getting slow for Jest to run
It may be that Jest sharding takes care of this, however, it was also a bit unwieldy in terms of human readability, scrolling that much just causes me to want to die, and this new file is where I'll be adding a bunch of tests around resize behavior in my next PR
# Conflicts: # packages/@react-spectrum/table/test/Table.test.js
snowystinger
dismissed stale reviews from devongovett, reidbarber, and LFDanLu
via
e9e1e6c
|
Build successful! 🎉 |
|
Build successful! 🎉 |
5 participants
Closes
Removes the deprecated way of doing column sizing.
Splits tests out that are concerned with layout, this will speed up Jest a little bit as well as make it easier to navigate to a test of interest. No new tests were added.
This part of the refactor fulfills:
Open questions:
Do we care that this is a breaking change to TableLayout? Nope, it's undocumented and really only used internally
Do we still want a feature flag? we are doing full release
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: