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 timeouts from pickselect #127

Closed
wants to merge 4 commits into from
Closed

Conversation

aurbano
Copy link

@aurbano aurbano commented Nov 18, 2016

Not sure if this is an issue for other people, but having calls to setState(...) inside of timeouts in the Pickselect component would trigger the typical error:

Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the undefined component.

The timeouts were required to know whether clicks were happening inside the component or outside, for hiding the dropdown menu on blur.

To solve this it uses an extra event on DropdownMenuItem "onWillFocus" which indicates whether another element is going to be focused. This way the DropdownMenu knows before receiving the blur event.

This works well for us so I figured it would be nice to contribute it back.

@ButuzGOL
Copy link
Contributor

@stomita please check

@stomita
Copy link
Collaborator

stomita commented Nov 19, 2016

I'm not sure the approach bringing onWillFocus to the menu item always works correctly. When it have a focusable child element inside of the menu, it will not fire focus event to the desired element. (Imagine the DateInput which has many focusable elements inside the menu).

I understand the error of setState when component is already unmounted, but the approach I think better is canceling the setState logic when component has unmounted, as shown in following article.
https://facebook.github.io/react/blog/2015/12/16/ismounted-antipattern.html

@aurbano
Copy link
Author

aurbano commented Nov 19, 2016

@stomita yeah that makes sense - since we haven't used all components I've done testing manually with the ones we used, so can't talk about the date input.

Even if the isMounted() call is an "antipattern" it would be a cleaner solution, I'll give it a go soon

@stomita
Copy link
Collaborator

stomita commented Nov 21, 2016

@aurbano I would like to confirm that the article I specified is not recommending the isMounted() usage, but says the same thing can be done using lifecycle methods. I suspect the latest ES6 based component might not have the isMounted() call.

@aurbano aurbano closed this Mar 1, 2022
@aurbano aurbano deleted the timeouts branch March 1, 2022 16:47
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