display end-of-day, ILM-based events in the calendar #8940
Conversation
✅ Deploy Preview for ilios-frontend ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
stopfstedt
force-pushed
the
6631-ilm-calendar-display
branch
2 times, most recently
from
fcdd859 to
6820286
Compare
these new attrs default to the given event's start- and end-date. for end-of-day ILMs, these attrs are calculated, in order to make them presentable in a calendar grid.
… offering calendars. since we're dealing with offering-based events only here, we can default these attrs to startDate and endDate directly. no edge-case logic needed.
stopfstedt
force-pushed
the
6631-ilm-calendar-display
branch
2 times, most recently
from
f337fef to
a00793f
Compare
stopfstedt
force-pushed
the
6631-ilm-calendar-display
branch
from
a00793f to
8a77b54
Compare
stopfstedt
requested review from
jrjohnson and
michaelchadwick
as code owners
jrjohnson
left a comment
jrjohnson
left a comment
There was a problem hiding this comment.
I'm good with this as an iterative change and it solves the problem nicely. I'm in full agreement that another refactor is needed for the Event. There are too many places where we're faking the shape of the object in both app and test code. We're also having to create luxon objects with the same data many times in many places and sometimes reading the original data as well for sorting and display. All this could be eased significantly with a base class that was more complete.
| // Multi-days are currently not shown in the calendar, instead they are displayed in a table below the calendar. | ||
| // To prevent this from happening, we pin the calendar display start-date to 11:45p and the end-date to 11:59p. | ||
| // [ST 2025/11/07] | ||
| const startDate = DateTime.fromISO(obj.startDate); |
There was a problem hiding this comment.
Readability tweak: could this be inside the if block and then, since it is ilm only in there, be dueDate. Because it's confusing on the next couple of lines to be doing startDate.set, even though luxon treats set as returning a new instance of the value, it's hard to read it that way. Looks like you're just setting a value on startDate twice.
There was a problem hiding this comment.
Oh, no probably not because then the hour and minute would be missing. Leaving this comment in anyway, wonder if there is a better luxon API to use here. Or if set doesn't mean to everyone else what it feels like to me?
| obj.isUserEvent = isUserEvent; | ||
|
|
||
| // The start and end date of the event, for display purposes on the calendar. See comment block below. | ||
| obj.calendarStartDate = obj.startDate; |
There was a problem hiding this comment.
I like this rename, in the followup to this when we build events as classes we should also drop outside access to startDate and endDate if possible.
3 participants
fixes ilios/ilios#6631
couple of notes:
calendarStartDateandcalendarEndDateare derived values and should really be getters. as a follow-up, make event objects based on an actual class, rather than using a plain JS object. then, we can make these attrs actual getters.data-test-attrs to the event notes in the calendar grid that would carry this information, which would allow us to the read them back out during testing. another follow-up item.