-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[scheduler] Add all day events #18940
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
Conversation
Deploy preview: https://deploy-preview-18940--material-ui-x.netlify.app/ Bundle size report
|
<div className="AllDayGridCells"> | ||
{days.map((day) => ( | ||
<div | ||
key={day.day.toString()} | ||
className="DayTimeGridAllDayEventsCell" | ||
aria-labelledby={`DayTimeGridHeaderCell-${day.day.toString()}`} | ||
role="gridcell" | ||
data-weekend={isWeekend(adapter, day) ? '' : undefined} | ||
/> | ||
))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a weird solution but I needed a way to render the grid (and colored backgrounds) behind the events. All day events can span across multiple columns, so they can't exactly be placed inside a specific cell. 🙈 If we agree on this solution (or a similar one) I can refactor the month view as well to respect the same structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would require some adjustment on the day grid drag and drop
We can start with this approach and see if it's problematic, but on Bryntum the events are rendered in the 1st cell with position: absolute
from what I saw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm checking Google Calendar too, they take a similar approach: the event is placed inside the first cell with a specific width set (screenshot 1). Also when looking at the month view, I was wondering how they account for space from previous-day events, and it seems like there's a hidden element involved.
This seems far from a simple implementation 🙃, especially if we want to support drag and drop properly, so we should think it through carefully 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK so if I understand correctly, to render an event that spans across multiple day, they render a visible event in the 1st cell that has position: absolute
and a width to overlap on the next cell(s).
And on the other cells, they render a hidden event.
I think that's a good approach.
There might be some complexity to optimize the layout, but we can keep that for later (if I have 3 events, on from on day 1, one on day 2 and one on day 1 to 2, then I want ot render the 1st and 2nd event on the same line and the 3rd event on its own line).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that’s exactly right, we’ve been pairing and trying things out, and we’ve explored this approach. It actually looks quite promising!
@noraleonte worked her magic and it’s almost there 👏 This strategy allows us to rely on our existing primitives and also makes it much easier to handle both single-day and multi-day events in the month view.
As you said, there are some complex edge cases, especially when events overlap, but we believe they’re solvable 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small update: I refactored this piece. The events are now rendered within the cell. When an all day event spans across multiple columns, the first column is the one that renders the event with a larger width (css black magic there 🙈 ), and the following ones render an invisible version of the event - could even be a div maybe. That part is there to be able to avoid overlapping and make sure the event are rendered in the right row.
I also made some changes to the selector @flaviendelangle. I need the information of which row the events start rendering on on each column to avoid the situation where the invisible event is rendered on the wrong row if there are no other event rendered above it.
I'll let you guys check out this approach and if we think it's good I'll refine this PR and implement the logic on the month view as well 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of small suggestions, in general looks amazing! IMO the approach looks correct 💯
allDayEvents = events.filter( | ||
(event) => | ||
event.allDay && | ||
(!event.resource || !visibleResources.get(event.resource)) && | ||
adapter.startOfDay(event.start) <= adapter.startOfDay(day) && | ||
adapter.startOfDay(event.end) >= adapter.startOfDay(day), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this selector is becoming too crowded 😆
I tried reworking it so that it returns the list of events that should be rendered in a given day.
Feel free to modify it if it doesn't suit your needs.
To handle all day events on the month view, you will have to set shouldOnlyRenderEventInOneCell
to true
and handle the spanning correctly.
On the agenda view it now renders the events in all of its days.
And we can clean the function that splits all day and non all day events (based on yesterday's discussion) depending on how we want to handle non-all-day but multi-days events.
For now I kept it in the DayTimeGrid.tsx
file for simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, with this approach we don't support #18828
To support it, we would need to do the split all day / non all day at the selector level.
I'll make a 2nd commit to allow a method for shouldOnlyRenderEventInOneCell
so that the selector can apply an event to all days when it will be rendered in the time grid but only to a single day when it will be rendered in the day grid.
It's not blocking though
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
4e842a0
to
8937114
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
… add-all-day-events
packages/x-scheduler/src/material/internals/components/day-time-grid/DayTimeGrid.css
Show resolved
Hide resolved
It might make sense to implement the all day events on the month view in a follow-up PR. This is becoming a bit bloated and on the month view we might bump into more edge cases 🤔 |
No problem to split it in two yeah 👌 |
@@ -6,6 +6,46 @@ export const defaultVisibleDate = DateTime.fromISO('2025-07-01T00:00:00'); | |||
const START_OF_FIRST_WEEK = defaultVisibleDate.startOf('week'); | |||
|
|||
export const initialEvents = [ | |||
{ | |||
id: 'allday-0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I would stick with "All day event A1" etc... type of name for consistency.
And more important: could you add an all day event that goes from the 30th to the 1st? To see if it correctly goes below the two "Conferences day" without creating a new line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I optimized the calculation of the row indexes because your suggestion helped me uncover some edge cases.
Here's what it looks like now:
Short answer: No, A2 does not go below B1. It renders the invisible cells in order. I think there are improvements to be made here:
- A2 should more efficiently use the space that is created below
- More importantly, C1, D1, D2, D3, and E1 could use the empty rows above. But there are things we would need to debate. For instance, which event takes precedence over the other? D1, D2 or D3? C1 move up? Does E1 go to the first row, or sticks after C1?
I'll explore all of these points in a follow-up.
))} | ||
</div> | ||
</div> | ||
<DayGrid.Root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screencast.2025-07-31.14.35.37.mp4
Is the scroll expected here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn, no, not expected. I'll look into it
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
ref={allDayHeaderWrapperRef} | ||
className={clsx('DayTimeGridGridRow', 'DayTimeGridAllDayEventsGrid')} | ||
role="row" | ||
data-weekend={lastIsWeekend ? '' : undefined} | ||
> | ||
<div | ||
className="DayTimeGridAllDayEventsCell DayTimeGridAllDayEventsHeaderCell" | ||
role="columnheader" | ||
> | ||
{translations.allDay} | ||
</div> | ||
<DayGrid.Row | ||
className="DayTimeGridAllDayEventsRow" | ||
role="row" | ||
style={{ '--column-count': days.length } as React.CSSProperties} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't contain one row inside another row, we have role=row
on line 121 and a nested one in 132 🤔
I think a good solution would be to remove the outer role="row"
, and then we can associate the cells with both headers using aria-labelledby
. That way, each cell can reference the "All day" row header and the day column header, even if the row header is rendered outside. This should keep the structure accessible and avoid nested row roles, which aren't valid, wdyt?
const previousEventRowPosition = getEventWithLargestRowIndexForDay(dayKey, daysMap); | ||
|
||
if (previousEventRowPosition + 1 > eventIndex + 1) { | ||
for (let i = 1; i < previousEventRowPosition + 1; i += 1) { | ||
if ( | ||
daysMap | ||
.get(dayKey)! | ||
.allDayEvents?.findIndex((eventInMap) => eventInMap.eventRowIndex === i) === | ||
-1 | ||
) { | ||
eventRowIndex = i; | ||
break; | ||
} | ||
} | ||
} else { | ||
eventRowIndex = previousEventRowPosition + 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const previousEventRowPosition = getEventWithLargestRowIndexForDay(dayKey, daysMap); | |
if (previousEventRowPosition + 1 > eventIndex + 1) { | |
for (let i = 1; i < previousEventRowPosition + 1; i += 1) { | |
if ( | |
daysMap | |
.get(dayKey)! | |
.allDayEvents?.findIndex((eventInMap) => eventInMap.eventRowIndex === i) === | |
-1 | |
) { | |
eventRowIndex = i; | |
break; | |
} | |
} | |
} else { | |
eventRowIndex = previousEventRowPosition + 1; | |
} | |
const allDayEvents = daysMap.get(dayKey)!.allDayEvents; | |
const usedIndexes = new Set(allDayEvents.map((event) => event.eventRowIndex)); | |
eventRowIndex = 1; | |
while (usedIndexes.has(eventRowIndex)) { | |
eventRowIndex += 1; | |
} |
I was testing this locally and I think we could simplify and slightly optimize it by using a Set to track used indexes, instead of looping and calling .findIndex()
each time.
It avoids the need for getEventWithLargestRowIndexForDay
and makes the logic a bit easier to follow. Let me know what you think because I might be missing something 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonus track! After that we could also extract that section into a function like
function getEventRowIndex(
event: CalendarEvent,
day: SchedulerValidDate,
days: SchedulerValidDate[],
daysMap: Map<string, { allDayEvents: CalendarEventWithPosition[] }>,
adapter: Adapter,
): number {
const dayKey = adapter.format(day, 'keyboardDate');
const eventFirstDay = adapter.startOfDay(event.start);
// If the event starts before the current day, we need to find the row index of the first day of the event
const isBeforeVisibleRange =
adapter.isBefore(eventFirstDay, day) && !adapter.isSameDay(days[0], day);
if (isBeforeVisibleRange) {
const firstDayKey = adapter.format(
adapter.isBefore(eventFirstDay, days[0]) ? days[0] : eventFirstDay,
'keyboardDate',
);
// Try to find the row index from the original event placement on the first visible day
const existingRowIndex = daysMap
.get(firstDayKey)
?.allDayEvents.find((item) => item.id === event.id)?.eventRowIndex;
return existingRowIndex ?? 1;
}
// Otherwise, we just render the event on the first available row in the column
const usedIndexes = new Set(
daysMap.get(dayKey)?.allDayEvents.map((item) => item.eventRowIndex) ?? [],
);
let i = 1;
while (usedIndexes.has(i)) {
i += 1;
}
return i;
}
let eventDays: SchedulerValidDate[]; | ||
if (shouldOnlyRenderEventInOneCell) { | ||
if (adapter.isBefore(eventFirstDay, days[0])) { | ||
eventDays = [days[0]]; | ||
} else { | ||
eventDays = [eventFirstDay]; | ||
} | ||
} else { | ||
eventDays = days.filter((day) => | ||
isDayWithinRange(day, eventFirstDay, eventLastDay, adapter), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to extract this too, to a function called getEventDays
or similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nice job on this! 💙 You’ve handled a pretty complex piece of logic with care and it’s looking really solid, seriously, amazing work. I’ve suggested a small improvement around how the eventRowIndex
is computed and left a few optional ideas on extracting logic to make it more readable, but not a blocker!
One thought that might help with readability long-term: since the logic is quite dense, maybe it could be worth adding some high-level comments like // Step 1: Sort events
, // Step 2: Blabla
. It might make it easier for the next person to follow the flow at a glance.
Again, awesome job, you clearly put a lot of care into this! 🙇♀️ 💙
@@ -99,7 +86,7 @@ export const selectors = { | |||
const dayKey = adapter.format(day, 'keyboardDate'); | |||
daysMap.set(dayKey, { events: [], allDayEvents: [] }); | |||
} | |||
|
|||
// STEP 1: Sort events by start date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💙 This helps a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks amazing to me!! ✨ So much cleaner, again, thanks for taking care of this and good job 🥇
Preview link
Closes #17713
