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

Fixes #49 fix console error by letting event bubble to helper function #50

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

quangta93
Copy link

No description provided.

@MilosRasic MilosRasic requested review from MilosRasic and removed request for darktasevski September 2, 2019 10:09
@MilosRasic
Copy link
Contributor

Thanks for the PR @quangta93. Good catch, but the solution doesn't eliminate the core of the issue, so it fixes the problem with clicking on the dropdown icon but possibly breaks another use case: custom handling of dropdown item click. The reason why it works is removing the event.stopPropagation() call ReactSvgIcon component. The event will now bubble up but if a custom handleClick() is passed it will never get called.

I think a simple solution here would be to add a guard to onClick() function in ReactSvgIcon instead of removing it. Something like:

	const onClick = e => {
		if (!handleClick) return;

		e.stopPropagation();
		return handleClick();
	};

Feel free to prove me wrong if that's the case. It's been a while since I've used or looked at this code.

@quangta93
Copy link
Author

@MilosRasic I believe my changes don't break custom handling of dropdown item click. What I did was lifting the handling of click events from ReactSvgIcon into helpers/index.js. event.stopPropagation() is still called inside helpers/index.js and the existence of handleClick is also checked inside that function. I think ReactSvgIcon is simply a "dumb" component so it should not interrupt the event bubbling process i.e. click events should bubble to higher level.

In addition, all SVG icons are now using onClick props, which is a more conventional way of naming things.

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