Remove schedule events duplicate requests and ignore window.resize #4307
Conversation
maskin25
changed the title
| useEffect(() => { | ||
| window.addEventListener('resize', onResize); | ||
| return () => { | ||
| window.removeEventListener('resize', onResize); | ||
| }; | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
was there maybe a valid reason why we called onHide on window resize? 🤔
There was a problem hiding this comment.
Screen.Recording.2024-05-10.at.11.09.30.mov
I found the only case when window resizng can be an issue (see video attached), in that case user can just resize window back, it's better than just close the modal on any resize
There was a problem hiding this comment.
IMO we should add some calculations if we remove the resize handler. When that happens it's not very clear for the user that the resize triggered that behavior, and we will run into escalations again. Either we
- close the modal on resizing (no one on prod environment is expected really to open the devtools)
- OR figure out boundaries for the modal on each
resizeevent, and eventually, if it doesn't fit, you'll still need to close it, so it's basically the first option with 1 extra step
There was a problem hiding this comment.
Either way I think some implementation needs to be added when the browser is being narrowed, otherwise users will run into scenario where they cannot close/submit the modal anymore due to vertical/horizontal restrains.
| useEffect(() => { | ||
| refreshEvents(scheduleId); | ||
| }, [selectedTimezoneOffset]); |
There was a problem hiding this comment.
so this was refetching events if you changed the timezone dropdown and this will no longer be the case? Is there any negative consequences of that?
There was a problem hiding this comment.
Since your comment I've added refreshing schedules events on TimezoneSelect onChange callback, it settles things
maskin25
added
pr:no public docs
| useEffect(() => { | ||
| window.addEventListener('resize', onResize); | ||
| return () => { | ||
| window.removeEventListener('resize', onResize); | ||
| }; | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
IMO we should add some calculations if we remove the resize handler. When that happens it's not very clear for the user that the resize triggered that behavior, and we will run into escalations again. Either we
- close the modal on resizing (no one on prod environment is expected really to open the devtools)
- OR figure out boundaries for the modal on each
resizeevent, and eventually, if it doesn't fit, you'll still need to close it, so it's basically the first option with 1 extra step
| useEffect(() => { | ||
| window.addEventListener('resize', onResize); | ||
| return () => { | ||
| window.removeEventListener('resize', onResize); | ||
| }; | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
Either way I think some implementation needs to be added when the browser is being narrowed, otherwise users will run into scenario where they cannot close/submit the modal anymore due to vertical/horizontal restrains.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
What this PR does
Remove schedule events duplicate requests
Which issue(s) this PR closes
Checklist
pr:no public docsPR label added if not required)release:). These labels dictate how your PR willshow up in the autogenerated release notes.