Skip to content
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

fix(list): RemoveItem correctly removes item when filtering #638

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prithvijj
Copy link

@prithvijj prithvijj commented Oct 15, 2024

Changes

  • Added in a fix that allows the correct item to be removed when the list is being filtered
  • Previously, when it was in the filtering state and the RemoveItem method was called, it would remove the item from the filteredItems based on the index, however it would use the same index and remove from unfiltered items too, which is inconsistent since the index associated to the filteredItems may not be the same as the index of the unfiltered items
  • Hence to fix this, when it is in the filtering state and the RemoveItem method is called, it retrieves the index index of the unfiltered items that's available in the filteredItems and then correctly removes the item from filteredItems and unfiltered items

Testing Notes

Previously

Regular view
image

Press / and Filter for bananas

image

image

Press x
Shows that Artichoke was removed, and bananas wasn't

image


Now

Regular view
image

Press / and Filter for bananas

image

Press x
Shows that bananas was removed, which is expected

image

Other Notes

Related to #632

  • Not sure how to write tests for it? Looking for help on that

- Added in a fix that allows the correct item to be removed
  when the list is being filtered
- Previously, when it was in the `filtering state` and the `RemoveItem`
  method was called, it would remove the item from the `filteredItems`
  based on the index, however it would use the same index and remove
  from `unfiltered items` too, which is inconsistent since the index
  associated to the `filteredItems` may not be the same as the index
  of the `unfiltered items`
- Hence to fix this, when it is in the `filtering state` and the
  `RemoveItem` method is called, it retrieves the index index
  of the `unfiltered items` that's available in the `filteredItems`
  and then correctly removes the item from `filteredItems` and
  `unfiltered items`
@meowgorithm
Copy link
Member

Thanks, @prithvijj. Do you mind adding some code to help illustrate what's being fixed?

@prithvijj
Copy link
Author

Thanks, @prithvijj. Do you mind adding some code to help illustrate what's being fixed?

Hey @meowgorithm , kept it in draft, since I'm like adding testing notes as I type :)

Shall request a review here and in the discord when it's ready!

@meowgorithm
Copy link
Member

Sounds good; here is fine. Keep in mind that we have a lot of inbound PRs and issues and it may take a moment to get this one as we manage intake — however example codes definitely helps expedite the process.

@prithvijj prithvijj marked this pull request as ready for review October 15, 2024 14:22
@prithvijj
Copy link
Author

Sounds good; here is fine. Keep in mind that we have a lot of inbound PRs and issues and it may take a moment to get this one as we manage intake — however example codes definitely helps expedite the process.

Yepp, aware of all the inbound pending PRs

PR is ready now, so feel free to check it out whenever convenient

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants