Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

fix: no onSelect event when same item selected #102

Merged
merged 3 commits into from
May 24, 2021

Conversation

iambumblehead
Copy link
Owner

No description provided.

@iambumblehead iambumblehead requested a review from lwhiteley May 22, 2021 22:09
@github-actions
Copy link

github-actions bot commented May 22, 2021

🎊 PR Preview c79b742 has been successfully built and deployed to https://iambumblehead-react-dropdown-now-preview-pr-102.surge.sh

🕐 Build time: 173.451s

🤖 By surge-preview

@iambumblehead
Copy link
Owner Author

@lwhiteley changes here are for this ticket #96

@iambumblehead
Copy link
Owner Author

@lwhiteley the more I think about it... the current behaviour before the changes here seems correct. Maybe we should close the issue and update the test to verify that onSelect is called even when the value is the same.

What do you think?

@lwhiteley
Copy link
Collaborator

Think the issue is that the events should not be triggered when there is no user interaction

@iambumblehead
Copy link
Owner Author

@lwhiteley I had the same impression at first and I even added a test that rendered two dropdowns referencing the same value --changing one dropdown did not change the value or trigger events at the other dropdown. I could re-add that test.

@iambumblehead
Copy link
Owner Author

@lwhiteley upon second thought, the two-dropdowns tests used a primitive value "one" and maybe using an object value instead triggers the onChange event.

@iambumblehead
Copy link
Owner Author

I'll add the two-dropdowns test in a few hours

@iambumblehead
Copy link
Owner Author

@lwhiteley please review

@iambumblehead
Copy link
Owner Author

@lwhiteley I was able to reproduce the issue. Changing the value of the second dropdown triggered onChange at the first dropdown. To resolve it, the dropdown reads the event target to detect if the event originates at the dropdown.

@iambumblehead
Copy link
Owner Author

There could issues if the dropdown later needed to support keyboard selection and input, but if you're OK with the current change I will be too.

@iambumblehead
Copy link
Owner Author

do you use Matrix @lwhiteley? https://matrix.org/clients/

@lwhiteley
Copy link
Collaborator

do you use Matrix @lwhiteley? https://matrix.org/clients/

never heard of it. what would be the purpose of it?

Copy link
Collaborator

@lwhiteley lwhiteley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can go with this for now and maybe review the entire composition in a next major version

@lwhiteley
Copy link
Collaborator

lwhiteley commented May 24, 2021

never heard of it. what would be the purpose of it?

actually seems i have used it without knowing. i used gitter.im before. You can create rooms for git repositories

@iambumblehead
Copy link
Owner Author

@lwhiteley this page has a little blurb describing matrix https://tatsumoto-ren.github.io/blog/join-our-community.html

Matrix is great because it's completely libre and federated, and you don't need an email address, or a phone number to make an account. Just type your name and a password, and you're in! Before you join, install a Matrix client and register an account on any server that is not matrix.org.

I know about it because there are clients for ubuntu touch and gnome3, which I use on my phone and desktop. Also, I like messaging applications and have been building a service that follows the matrix spec in many parts. I would like to be your friend on Matrix or another messaging system.

@iambumblehead iambumblehead merged commit 2bf0333 into master May 24, 2021
@iambumblehead iambumblehead deleted the do-not-onchange-unmounted branch May 24, 2021 17:34
@lwhiteley
Copy link
Collaborator

can chat privately on https://gitter.im/

and discuss further

@iambumblehead
Copy link
Owner Author

I signed in there with my github proifile and if I type "lwhi" or "whit" it doesn't auto-complete "lwhitely" there.

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

Successfully merging this pull request may close these issues.

3 participants