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: Corrects the events passed into each day #97

Conversation

scottlovegrove
Copy link
Contributor

Ok, so what I think the problem here was, there were two places where the events for the day were being calculated, rather than just sticking to one. I've moved things around a little so that there is a single source of truth for events for the day, and that's calculated in the CalendarProvder, which is where I think it should be done. As such, I have removed some of the code from the EventIntersectionProvider, because that code seemed primarily to be about showing/hiding events, rather than what events it had.

I've tested this with my own consuming project and with these changes, everything works as expected when I filter events out.

Fixes: #95

@bruceharrison1984
Copy link
Owner

I've moved things around a little so that there is a single source of truth for events for the day, and that's calculated in the CalendarProvder, which is where I think it should be done.

That is a good call. I noticed that when I was reworking some stuff for the start of week changes. Nearly all the logic could be pulled up into the calendar provider and calculated there instead of within the components.

To be honest, the eventing was added as sort of an after thought so it's good to see it getting some attention.

@scottlovegrove
Copy link
Contributor Author

I'm unsure why the dateAdapter tests are failing, they run just fine for me locally. Perhaps it's a timezone thing for where the tests are being run?

@bruceharrison1984
Copy link
Owner

Perhaps it's a timezone thing for where the tests are being run?

That may be the case. The tests failing during CI are correctly failing based on the criteria. There hasn't been an issue with tests regularly failing previously.

https://github.com/bruceharrison1984/Schedulely/actions/runs/5630688949/job/15256926107#step:6:32

If you look. at the test case you added, you explicitly defined a Zulu time. The other test cases utilize new Date, which makes them native to whatever TZ they are created. You might try changing your test and see if that fixes the issue.

@scottlovegrove
Copy link
Contributor Author

a Zulu time

Never heard of this before, so just looked it up 😄 I've always just called it a UTC time :)

@bruceharrison1984
Copy link
Owner

It's still UTC, but a specific format. I once worked on a project that dealt with TZs and the confusion between general UTC and a Zulu formatted one wasted hours of time in meetings 🤣 so now it just stuck with me.

If I can find some time today or tomorrow I'll check this out and hopefully get it merged in.

Thanks!

@bruceharrison1984 bruceharrison1984 merged commit 6e5574f into bruceharrison1984:main Jul 22, 2023
3 checks passed
@scottlovegrove scottlovegrove deleted the scottl/fix-events-on-days branch July 22, 2023 21:11
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.

Events passed to DayComponent aren't recalculated if events change but month stays the same
2 participants