-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(RAC): use the correct approach to retrieve the items array #9539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
`collection.getKeys()` doesn’t guarantee the correct order.
reidbarber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is a failing test.
Can you say more about what issue this was causing?
|
@reidbarber
|
| return itemNode?.type === 'item' ? itemNode : null; | ||
| } | ||
| ).filter(node => node !== null); | ||
| const cachedItemNodes = Array.from(collection).filter(itemNode => itemNode.type === 'item'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the typo causing the test failure. This should be using cachedCollection.current.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was a typo. But technically, it shouldn't impact single-item deletes because only the cachedCollection length is used to gate multi-item deletion. I've applied the fix, but the test is still failing.
|
Thanks for the PR, I've opened a new one #9545 that fixes the test and adds a test instead of storybook so we are less likely to regress. There was a little more complexity to the keys in a collection with section and the logic was also flawed moving focus by all the items removed instead of only the ones before. I think we were getting lucky earlier with this test passing, though it was also just a really hard to follow test that did a bunch of things that wouldn't happen in real life. |
collection.getKeys()doesn’t guarantee the correct order.Closes
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: