fix(#1885) Making ListLayout updateItemSize early return if layoutInfo is undefined #1920
Conversation
fixes issue 1885, the checkbox cell no longer has layoutInfo when selectionMode goes from multiple to none
LFDanLu
requested review from
dannify,
devongovett and
snowystinger
as code owners
|
Build successful! 🎉 |
| // If no layoutInfo, item has been deleted/removed. | ||
| if (!layoutInfo) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Alternative/supplemental fix: maybe change https://github.com/adobe/react-spectrum/blob/main/packages/@react-stately/virtualizer/src/Virtualizer.ts#L907 to something like
let changed = !this.layout.getLayoutInfo(key) || this.layout.updateItemSize(key, size);
aka if that key doesn't have layout info, don't bother calling updateItemSize and perform a relayout (or maybe don't relayout? Think it might be "don't perform a relayout" in this case since the test complains about the max update depth being exceeded when I set return true here)
|
Build successful! 🎉 |
There was a problem hiding this comment.
LGTM, tested in Chrome
i do see your other question, I don't have an opinion right now because it wouldn't really be informed. I'm happy to see how this one goes. Though if you want to elaborate on the two approaches (pros/cons?) maybe that'd help me form more of an opinion
|
The pro of adding/using the virtualizer change would be that it is perhaps a more central change (it is the one calling layout.updateItemSize). However, it makes the assumption that if |
|
Build successful! 🎉 |
Closes #1885
✅ Pull Request Checklist:
📝 Test Instructions:
Test steps:
🧢 Your Project: