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

🐛 Fix: All day event doesn't disappear after changing the week - Attempt #2 #185

Merged
merged 5 commits into from
Dec 14, 2024

Conversation

murilo9
Copy link
Contributor

@murilo9 murilo9 commented Dec 12, 2024

This is a better, non-style-level fix for #178

As you suggested, I've added some logic into useDraftUtil.ts:submit to check if the updated event will be rendered inside current view. If not, it marks the event to be removed from the store after the update. Now once we save an event with dates for a different week, it immediately disapers from the screen.

All automated tests have passed.

However, the "flicking" event with full width still persists for some milliseconds once you change weeks. I'm not sure yet what is the cause of this, but I will let the PR open while I give a look on this.

@tyler-dane tyler-dane self-requested a review December 12, 2024 11:38
@tyler-dane tyler-dane linked an issue Dec 12, 2024 that may be closed by this pull request
1 task
@tyler-dane
Copy link
Contributor

"However, the "flicking" event with full width still persists for some milliseconds once you change weeks. I'm not sure yet what is the cause of this, but I will let the PR open while I give a look on this."

Can you provide steps to replicate this?

Are you referring to the sidebar's re-rending (including the loading spinner)? If so, that's a known and unrelated issue. If it's something else related to the main-grid, then I'd like to replicate it before merging this

Copy link
Contributor

@tyler-dane tyler-dane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better so far.

Would like more clarification on the flickering you mentioned before QA-ing further.

packages/web/src/ducks/events/event.types.ts Outdated Show resolved Hide resolved
@murilo9
Copy link
Contributor Author

murilo9 commented Dec 12, 2024

How to reproduce the "flickering" event bug:

  1. Add an all-day event in a different week.
  2. Move the view to that week, so you can see the all-day event. It should render properly.
  3. Move back to the previous week, you'll note that an all-day event with the duration of the whole view quickly shows (for less than a second) then disappears.

I'm not sure what is causing that but I suspect it might be something to do with the order of the steps in editEvent saga.

Update: my suspiction is that when we change the current week, CalendarView re-renders, triggering re-renders in all its component tree until reaching AllDayEvents component, so only then AllDayEvents re-calculates the correct value of its allDayEvents variable in order to display the new week's events. As that series of re-renders take some milliseconds to complete, we still see the old events very briefly. I've tried moving the allDayEvents variable to CalendarView and prop drill it down to the AllDayEvents component, but the flicking persists. I'm out of ideas on how to solve that now =(

@tyler-dane
Copy link
Contributor

Thanks for including the detailed steps to reproduce it. Since that behavior exists in main now, we don't need to block this PR. I create a new bug to address it in the future: #186

Doing a little more QA on this one, but it's in pretty good shape. Please feel free to continue working on your other issue while I continue testing/tweaking

murilo9 and others added 5 commits December 14, 2024 06:14
use shouldRemove prop to determine whether to delete or edit (was previously calling both)
this'll reduce the payload needed for when the sidebar calls this action (which doesn't need to use this concept of removing)
@tyler-dane tyler-dane force-pushed the fix/issue#178-attempt2 branch from 3da870a to 6e20ac3 Compare December 14, 2024 12:16
@tyler-dane tyler-dane merged commit 4865fad into SwitchbackTech:main Dec 14, 2024
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.

All day event doesn't disappear after changing the week
2 participants