fix(Popup): execute onClose when Popup closes on scroll
#2182
Conversation
|
I am not sure what is breaking the tests. It passed locally and the commit name follows the guidelines. Help? |
| this.setState({ closed: true }) | ||
|
|
||
| eventStack.unsub('scroll', this.hideOnScroll, { target: window }) | ||
| setTimeout(() => this.setState({ closed: false }), 50) | ||
|
|
||
| this.handleClose(e) |
There was a problem hiding this comment.
@mkarajohn Did you investigate what closed value in state actually does? Why it becomes false after 50ms? This looks confusing to me.
There was a problem hiding this comment.
Well, I I saw it reopens the modal, even though I do not understand the reason as to why it does so.
From my understanding of the docs, and from the visual feedback, when the modal closes on scroll, it closes for good.
Could you please explain to me?
There was a problem hiding this comment.
Seems it is only used in one place, render. If true, the trigger is rendered directly, else, the Portal is rendered with the trigger set:
if (closed) return trigger
// ..snip
return (
<Portal...I can't see why a timeout is added here?
Also, I've been racking my brain as to why we have an error logged regarding setState being called on an unmounted component, and I believe this is the issue :) There is no cancelling of this timeout when the Popup unmounts, therefore, setState can still be called 50ms after unmount.
This looks like it might be a hack to "reset" the portal. By rendering the trigger only, there is no Portal so it appears to have closed. Then, by nearly immediately (50ms?) rending the Portal again this ensures the Portal behavior is added to the trigger again. Future click/hover/etc will re-open the Portal.
I would have to investigate a solution to have a solid answer, but my suggestion would be to explore moving Popup to an AutoControlledComponent. Then, in hideOnScroll, we likely need to this.trySetState({ open: false }) without a timeout. This way, we are attempting to immediately close the Popup on scroll.
There was a problem hiding this comment.
Also, I've been racking my brain as to why we have an error logged regarding setState being called on an unmounted component, and I believe this is the issue :) There is no cancelling of this timeout when the Popup unmounts, therefore, setState can still be called 50ms after unmount.
This was supposed to be closed here. It seems like it's been undone or something.
So, what about the initial concern that this PR addresses? The fact that the onClose is not called when the Popup closes because of scrolling?
There was a problem hiding this comment.
So, what about the initial concern that this PR addresses? The fact that the onClose is not called when the Popup closes because of scrolling?
Yes, any state change due to user interaction should fire callbacks. This would include scroll. We only skip callbacks when state is changed through props, such as toggling the open prop.
Codecov Report
@@ Coverage Diff @@
## master #2182 +/- ##
==========================================
- Coverage 99.73% 99.65% -0.08%
==========================================
Files 151 151
Lines 2624 2624
==========================================
- Hits 2617 2615 -2
- Misses 7 9 +2
Continue to review full report at Codecov.
|
|
I've retrigged the build, now it passes |
layershifter
changed the title
onClose when popup closes on scroll
layershifter
changed the title
onClose when popup closes on scrollonClose when Popup closes on scroll
|
@mkarajohn Let's add a test that asserts that |
…React into execute_onclose_when_hideonscroll
There was a problem hiding this comment.
@mkarajohn Thanks for PR. I've added a missing test and simplified existing. I will investigate the case with closed during the refactor in future
|
Released in |
I believe when a popup closes on scroll the
onClosehandler should be called, since the popup closed.This PR makes it so.