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

Remove duplicate handler for indicator #5085

Conversation

rkulinski
Copy link
Contributor

@rkulinski rkulinski commented Feb 28, 2022

I have no idea why two different handlers should be applied for single action. The approach with two handlers was forcing to call stopPropagation which was causing this issue to occur
#5050

@changeset-bot
Copy link

changeset-bot bot commented Feb 28, 2022

🦋 Changeset detected

Latest commit: 7fc125e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
react-select Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 28, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7fc125e:

Sandbox Source
react-codesandboxer-example Configuration

Copy link
Collaborator

@Rall3n Rall3n left a comment

Choose a reason for hiding this comment

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

Removing the dedicated event handler for the DropdownIndicator component introduces breaking changes and also breaks interaction with the component when using the prop openMenuOnClick:

  • setting openMenuOnClick={false} does not open the menu when clicking on the indicator
  • additionally setting isSearchable={false} shows no indication of interaction with the component upon clicking the indicator (resembles an <select readonly /> element)

Even though both handlers do the same actions, they react in different ways to the interaction. One should not be removed on the basis that they both end in the same behavior. Instead, a way should be found to replace stopPropogation without changing current behaviour.

@rkulinski
Copy link
Contributor Author

@Rall3n got it, I wasn't able to find any difference in how it was working, but I'm not that familiar with library code so I missed some edge case scenarios. I'll try to somehow get rid of stopPropagation, but I'm not sure if this will be easy.

@rkulinski rkulinski force-pushed the share-click-handler-for-dropdown-and-select branch from 680783c to 7fc125e Compare March 1, 2022 14:56
@rkulinski rkulinski requested a review from Rall3n March 1, 2022 15:02
@rkulinski
Copy link
Contributor Author

rkulinski commented Mar 1, 2022

@Rall3n I wasn't able to find any better and easier workaround for this with how components are structured. Some snapshots tests are failing and ofc branch name will have to be changed, but wanted to get a feedback on solution itself.

@rkulinski
Copy link
Contributor Author

Closing in favor of #5089

@rkulinski rkulinski closed this Mar 2, 2022
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.

None yet

2 participants