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

Skip extra item selection events when clicking an option #110

Closed
wants to merge 3 commits into from

Conversation

jlw
Copy link
Contributor

@jlw jlw commented Mar 14, 2024

While working on #108, I noticed some odd behavior around item selection that only manifested as a bug with the multiple selection. I simply added some extra conditionals in that PR because I hadn't yet worked out any clean fixes.

The first commit here addresses only the very first odd behavior - when I click on an option in the list, the first item is incorrectly selected before resolving on the actual option I clicked.

I verified this by adding the following:

debugging

which resulted in:

debugging output


The second commit here addresses both of the de-selection and re-selection events, seen here originally in a bit more detail:

reselections

and now has been removed without breaking any system tests (and passing minimal manual testing):

no-more-reselection

@jlw jlw changed the title Skip extra item selection when clicking an option Skip extra item selection events when clicking an option Mar 14, 2024
@josefarias
Copy link
Owner

Great catch @jlw thanks! I think this points to a larger problem in the codebase.

The current design couples certain things – like filtering and selection. Conditionals will keep proliferating unless we nip this in the bud.

I'm going to attempt a refactor I've had on the back burner for a while. That should fix this, as well as make our work in #106 easier. Although it might cause some minor merge conflicts. But we'll figure it out.

Gonna keep this open in case that refactor doesn't pan out. Working on it now.

@josefarias
Copy link
Owner

@jlw I finished the refactor in #111. That should've also fixed the issues you're seeing here. Closing for now.

Sorry about the merge conflicts in the other PR. I'll help you get those sorted tomorrow.

@josefarias josefarias closed this Mar 15, 2024
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