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

Allow event bubbling for dropdown click #5089

Conversation

rkulinski
Copy link
Contributor

@rkulinski rkulinski commented Mar 2, 2022

Bit explicit solution, but with the architecture I think it's the best one.

The approach with two handlers is forcing to call stopPropagation which was causing this issue to occur
#5050

@changeset-bot
Copy link

changeset-bot bot commented Mar 2, 2022

🦋 Changeset detected

Latest commit: 02b794b

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 Mar 2, 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 02b794b:

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.

I don't think setting a hard-coded className to perform a target check on is the best alternative.
The className could easily be removed using the components prop, which would result in this check being rendered useless.

There are two approaches which I think would be better:

  1. Setting an internal flag to skip the handler (similar to openAfterFocus), or
  2. Using event.defaultPrevented to skip the handler if preventDefault has been called. This should work because both indicators and the control use the same event for their handlers.

@rkulinski
Copy link
Contributor Author

@Rall3n yeah. Actually after a second thought that could be a problem if someone used same class name. I think internal flag is the best option.

@rkulinski rkulinski requested a review from Rall3n March 2, 2022 16:34
@rkulinski
Copy link
Contributor Author

Hey @Rall3n, would this PR be okay to merge?

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

@rkulinski Looks good, but after some consideration, I have come to the conclusion that a flag is not necessarily the best way to achieve this in terms of extensibility.

Theoretically, someone could replace the event handler using a custom component, or someone could add their own indicator. In those cases, you can't set the flag because it's not reachable in either of those.

Using event.preventDefault() and event.defaultPrevented would be more independent and much better for customization.

I apologize if this might be inconvenient for you.

packages/react-select/src/Select.tsx Outdated Show resolved Hide resolved
@rkulinski
Copy link
Contributor Author

@Rall3n please take a look now.

@Rall3n
Copy link
Collaborator

Rall3n commented Apr 3, 2022

It seems like the status are not being completed or even started on commit. I don't know when it started to break.

I also don't know how to force a restart, and I don't have the ability to merge despite required status not having finished.

@rkulinski I read that reopening the PR or rebasing should retrigger the status check. If not, the last resort would be to open a a new PR.

@rkulinski rkulinski closed this Apr 4, 2022
@rkulinski rkulinski reopened this Apr 4, 2022
@rkulinski rkulinski force-pushed the allow-event-bubbling-for-dropdown-click branch from 9a7fa33 to 02b794b Compare April 4, 2022 10:06
@rkulinski rkulinski closed this Apr 4, 2022
@rkulinski
Copy link
Contributor Author

Closing in favor of #5134

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