-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat(@wallet): update collectibles model only with changes #13544
feat(@wallet): update collectibles model only with changes #13544
Conversation
Jenkins BuildsClick to see older builds (16)
|
ca07f71
to
1759c77
Compare
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 good in general, just some smaller issues
for idx in oldIndicesToRemove: | ||
let updatedIdx = idx - removedItems | ||
self.removeCollectibleItem(updatedIdx) | ||
removedItems += 1 |
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.
Why are you counting the removedItems
?
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.
oldIndicesToRemove
holds the indices of the original list in ascending order. Every time we delete an item from the list, we should subtract 1 from the remaining indices in oldIndicesToRemove
. Instead of doing that I just keep track of how many items I've removed and do updatedIdx = idx - removedItems
.
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.
LGTM! And nice to have test included!!
1759c77
to
8c4cb41
Compare
8c4cb41
to
feac74d
Compare
@dlipicar i will check on failed tests on monday first thing. For now i see that some locator is missing for editing community, not sure what happens (it was passing today against master many times) |
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.
👍
feac74d
to
acce277
Compare
Fixes #13254
What does the PR do
Whenever a change in the list of owned collectibles is reported by status-go, we now update the model with the delta (added/removed items) instead of doing a full reset, which should help avoid doing unnecessary work in the SFPM.
This is a straightforward approach, and there's room for improvement still: