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

Added classWhiteList #12

Closed
wants to merge 2 commits into from
Closed

Added classWhiteList #12

wants to merge 2 commits into from

Conversation

gtaylor44
Copy link

Added an optional property (classWhiteList) to prevent onOutsideClick from firing if click event is within a specified whitelisted class. If classWhiteList is not specified, component behaves as normal. Gives the developer a little more control. Thanks for making this component open source.

Greg Taylor added 2 commits September 2, 2018 14:53
… ignored if the target node is a child of a class defined in the class white list.
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Using class names seems unreasonably restrictive; perhaps a predicate function that gets the event/node?

What’s your use case?

disabled: PropTypes.bool,
useCapture: PropTypes.bool,
display: PropTypes.oneOf(objectValues(DISPLAY)),
children: PropTypes.node.isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert all these indentation changes; we use 2 spaces, not 4.

@gtaylor44
Copy link
Author

gtaylor44 commented Sep 3, 2018

Use case:

Reason for using react-outside-click-handler in the first place is to close a popup menu when user clicks outside the menu. When the user clicks within the menu, it should remain open.

The issue is when the user clicks on the button that opens the menu while the menu is already open, the expected behaviour is to close the menu. In my case, the button can't be nested within <OutsideClickHandler> and the button needs to be excluded so the menu is not toggled twice (which has the effect of keeping menu open).

In summary... There are some parts of the UI I don't want onOutsideClick to be invoked. Specifying classes or raw nodes as you've suggested that you want to exclude from onOutsideClick can be useful in some situations.

@ljharb
Copy link
Collaborator

ljharb commented Sep 3, 2018

Would it not be easier to disable the opener when the menu is open, and re-enable it when it’s closed, preventing hue double-click scenario?

@gtaylor44
Copy link
Author

I did look at other ways to handle this without success. The onOutsideClick event happens before opener click event. That means the button is re-enabled for opening again before opener click event.

Anyway, if you believe this feature is superfluous, that's fine. I just thought it would be useful for some people with special use cases.

@ljharb
Copy link
Collaborator

ljharb commented Sep 3, 2018

I don’t mean listen for the opener click event - i mean rerender the opener as disabled and/or with a noop onClick handler.

@gtaylor44
Copy link
Author

I don't want to disable the component in any circumstance. I also don't need the additional complexity because this is needed in multiple places of the application. With the proposed changes, I've been able to achieve exactly what I want by just simply declaring the class name of the element I want ignored from onOutsideClick when it's clicked.

I've done a sanity check and from research I found a library similar to this one. There are people requesting the same feature. The referenced library (react-click-outside) gives the ability to intercept based on the target node.

The difference with this pull request is that it recursively checks parents of the target node to see if it's a child of the white-listed class.

kentor/react-click-outside#34
kentor/react-click-outside#16
kentor/react-click-outside#15
kentor/react-click-outside#8

@lencioni
Copy link
Contributor

lencioni commented Sep 4, 2018

@gtaylor44 thanks for opening this PR! I appreciate the effort you put into preparing this PR and exploring alternative approaches.

Since the Event object is passed to the event handler, can this functionality can be accomplished outside of this component in the event handler itself? If you are looking for a way to keep your code DRY and declarative, I think you could wrap this component with another one that adds this functionality.

With these things in mind, I'm not in favor of adding this complexity to this component to support this use case.

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

3 participants