-
Notifications
You must be signed in to change notification settings - Fork 815
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(selection list): update indexes after option removed #4464
fix(selection list): update indexes after option removed #4464
Conversation
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.
I tested your change with the example code from my issue and it works wonderfully. Thanks a lot! 💯
@@ -646,6 +646,11 @@ def _remove_option(self, index: int) -> None: | |||
option = self.get_option_at_index(index) | |||
self._deselect(option.value) | |||
del self._values[option.value] | |||
# Decrement index of options after the one we just removed. |
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.
Maybe a separate issue (or not an issue at all if I'm missing something), but this makes me wonder why the highlighted
index wouldn't be decremented if it appears below the removed option.
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.
Actually from a skim, I'm guessing it might be that the highlight is applied based on the ID of the option rather than the index. It's just converted to an ID via a validator.
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.
Thanks!
Fixes #4462.
I've added a basic test that the index is updated, but I'm not sure if you need a regression test that actually replicates the issue above?
Please review the following checklist.