chore(Popup): replace event-stack #4499
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4499 +/- ##
==========================================
+ Coverage 99.50% 99.53% +0.02%
==========================================
Files 186 186
Lines 3465 3463 -2
==========================================
- Hits 3448 3447 -1
+ Misses 17 16 -1 ☔ View full report in Codecov by Sentry. |
… rid of hacky timout that causes remounting Portal component.
|
Because of codecov's complaining about the uncovered setTimeout, I've decided to try to get rid of it as it seems to be hacky and not much clear what it does. As I understood, it causes the Portal component to remount for a moment (50ms) which causes it's inner state reset which causes visually closing the popup even if Popup's state "closed" is false. There were 2 cleaner ways how to do that IMO: fully control open state of Portal (which seemed to be not so easy) or let Portal handle the scroll event and close it's inner state and even propagate it via the onClose handler (because it's all there). It appears to me that this approach works well, no weird timeout, and even the functionality can be reused more. |
layershifter
changed the title
…event resubscribed every render
…ort older browsers
|
Congrats on merging your first pull request! 🎉🎉🎉 |
I think that removing event-stack might be better done incrementally as each the components are independent.
I've replicated that Popup only listens for scroll when the Popup is open and enabled.
I would prefer the whole useEffect hidden under the condition however that might require much more refactoring. This way it's cleaner to see what happened in the diff. Let me know the other way would be preferred.