Skip to content

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