[temporal] New Calendar component#1973
Conversation
flaviendelangle
added
the
type: new feature
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
commit: |
Bundle size report
|
github-actions
bot
added
PR: out-of-date
flaviendelangle
force-pushed
the
calendar-component
branch
4 times, most recently
from
51ba17e to
50a9781
Compare
flaviendelangle
requested review from
atomiks,
colmtuite and
michaldudak
as code owners
flaviendelangle
force-pushed
the
calendar-component
branch
from
6974886 to
f8e8271
Compare
|
I'm wondering if a template-based API (not fully composable, so instead of forcing users to render each week and day, they'd only specify a template and we will be responsible for rendering) wouldn't be a better choice here. I suppose (feel free to correct me, though) that in most (if not all) cases, users will simply map |
flaviendelangle
force-pushed
the
calendar-component
branch
from
2c490cd to
5e320ca
Compare
|
@michaldudak could you give me a basic JSX example of what it could look like? |
|
Something along these lines: <Calendar.DayGridBody className={styles.DayGridBody}>
<Calendar.DayGridRowTemplate>
{ (week, props) => <div {...props} className={styles.DayGridRow} /> }
</Calendar.DayGridRowTemplate>
<Calendar.DayGridCellTemplate>
{ (day, props) => <div {...props} className={styles.DayGridCell}><Calendar.DayButton className={styles.DayButton} /></div> }
</Calendar.DayGridCellTemplate>
</Calendar.DayGridBody>(alternatively, with a render prop instead of children) EDIT: I just realized this could be problematic with SSR, as templates would have to register themselves in their parent. We could work around this by looking for them with |
|
I have mixed feelings with the shortcuts on the DX here. In your example, how would people style the other nodes? (mostly I do agree with you that for bigger component, the fully composable approach becomes quite verbose. And that, for the Calendar, almost all app will have similar JSX if they use a fully composable approach. <CalendarGrid>
{(date) => <CalendarCell date={date} />}
</CalendarGrid>But it only works because they ship CSS by default. |
|
Another point, I decided to let the user map the cells, to be super flexible, but we could of course discuss replacing: <Calendar.DayGridRow value={week}>
{({ days }) =>
days.map((day) => (
<Calendar.DayGridCell value={day}>
<Calendar.DayButton />
</Calendar.DayGridCell>
))
}
</Calendar.DayGridRow>With: <Calendar.DayGridRow value={week}>
{({ day }) =>
<Calendar.DayGridCell value={day}>
<Calendar.DayButton />
</Calendar.DayGridCell>
}
</Calendar.DayGridRow>It's what React Aria does on their Date Field component |
|
@flaviendelangle Thanks for this, excited to see it! I had a quick first look over it. Chiming in on the recent feedback
I don't personally feel much pain in the JSX. On the contrary, I'm actually quite surprised at how terse it is. If there are new concepts to learn, or potential issues with SSR, or styling limitations, or content limitations, I'd opt for the usual normal compound pattern. If there are none of those limitations, it's worth considering.
This seems more promising to me. I prefer it without the Some questions/suggestions of my own:
Btw, lmk whenever you want some styling feedback. I assumed it's too early for that, so didn't bother. |
Feedback
Screen.Recording.2025-06-05.at.2.36.22.pm.mov
|
I will remove the map so that we can discuss this version 👍
I will check why
Sure
It did the split recently looking at #1963, but I'm very unsure
I just re-used what other components are doing when using
No big technical challenge here. If we are fine having 6 components it's good for me.
We let the date library handle that.
I did not explore that at all yet. Do you think it's needed on the initial PR or can it be kept for a follow up?
For me the current version make no assumption on the rendering layout to we could animate vertically. But I would need to explore and add experiments.
Not sure to understand this one.
Clearly bug
The accessibility is probably far from perfect for now. |
flaviendelangle
force-pushed
the
calendar-component
branch
from
12fcfac to
d4531d2
Compare
The naming was picked when the calendar supported month and year navigation. |
|
@atomiks React Aria writes "Next" as the aria label of the button that goes to the next month. |
|
@flaviendelangle For I wonder if the demos should set it manually so it becomes more obvious it needs to be localized |
Sorry for causing "commotion". I didn't restart my dev server. 🙈 |
… beyond min/max date limits
Claude Code reviewCode Structure1. In // CalendarViewport.tsx — this runs during render, not in an effect
if (currentContainerRef.current) {
if (isTransitioning) {
currentContainerRef.current.setAttribute('data-current', '');
// ...
} else {
for (const attribute of DATA_ATTRIBUTES) {
currentContainerRef.current.removeAttribute(attribute);
}
}
}This is a React anti-pattern. Render must be pure — side effects on DOM refs belong in 2. In React.createElement(previousContentNode.localName, {
className: currentContainerRef?.current?.className,
// ...
})
const ALLOWED_TAGS = new Set(['tbody', 'div', 'section']);
const tag = ALLOWED_TAGS.has(previousContentNode.localName)
? previousContentNode.localName
: 'div';3. The store class (~250 lines) handles value management, visible date management, navigation direction, day grid registration, date selection with timezone conversion, and observer wiring. This is a lot for one class. The // SharedCalendarStore.ts
private currentMonthDayGrid: Record<number, TemporalSupportedObject[]> = {};
public registerCurrentMonthDayGrid = (week, days) => {
const weekTime = this.state.adapter.getTime(week);
if (this.currentMonthDayGrid[weekTime] == null) {
this.currentMonthDayGrid[weekTime] = days;
}
return () => { delete this.currentMonthDayGrid[weekTime]; };
};This is called from 4. // CalendarDayGridHeader.tsx
const element = useRenderElement('thead', componentProps, {
ref: forwardedRef,
props: [{ 'aria-hidden': true }, elementProps],
});This hides the weekday column headers from screen readers entirely. The native table model associates 5. // LocalizationProvider.tsx
const adapterContextValue = React.useMemo(
() => ({ adapter: new TemporalAdapterDateFns({ locale: temporalLocale }) }),
[temporalLocale],
);This is correct in terms of memo deps, but the wrapping object Performance6. // useSharedCalendarDayGridBody.ts
const disabledIndices = React.useMemo(() => {
const output: number[] = [];
for (const itemMetadata of itemMap.values()) {
if (itemMetadata?.index != null && !itemMetadata.focusable) {
output.push(itemMetadata.index);
}
}
return output;
}, [itemMap]);For a 42-cell grid this is trivial, but the pattern creates a new array on every 7. Keyboard handler in // useSharedCalendarDayGridBody.ts — inside handleKeyboardNavigation
const gridDays = Object.values(store.getCurrentMonthDayGrid())
.flat()
.sort((a, b) => adapter.getTime(a) - adapter.getTime(b));This runs on every PageUp/PageDown keypress. Since 8. The examples and internal render-prop resolution all use weeks.map(children) // children receives (week, index, weeks)
// consumer does: key={day.toString()}
9. const getDateKey = (adapter: TemporalAdapter, date: TemporalSupportedObject) =>
adapter.format(date, 'localizedNumericDate');This is called inside 10. const isCurrent = React.useMemo(
() => adapter.isSameDay(value, adapter.now(adapter.getTimezone(value))),
[adapter, value],
);The deps are API Design Issues11. const isDisabled = useStore(
store,
selectors.isSetMonthButtonDisabled,
disabledProp, // passed as arg
targetDate,
disabledProp, // passed again as arg
);The selector signature is: (adapter, validationProps, isCalendarDisabled,
disabled: boolean | undefined,
targetDate: TemporalSupportedObject,
disabledProp?: boolean | undefined) => { ... }
12. export function useCalendarContext(): CalendarContext {
const store = useSharedCalendarRootContext();
const calendarPublicContext = useStore(store, selectors.publicContext);
return { ...calendarPublicContext, setVisibleDate: store.setVisibleDate };
}Every call returns a new object reference (due to the spread), so any component using this hook in a dependency array will re-run effects every render. Since return React.useMemo(
() => ({ ...calendarPublicContext, setVisibleDate: store.setVisibleDate }),
[calendarPublicContext, store.setVisibleDate],
);13. const executeAfterItemMapUpdate = React.useRef<(newMap: any) => void>(null);
const handleItemMapUpdate = (newMap: typeof itemMap) => {
setItemMap(newMap);
if (executeAfterItemMapUpdate.current) {
queueMicrotask(() => {
executeAfterItemMapUpdate.current?.(newMap);
executeAfterItemMapUpdate.current = null;
});
}
};This is a hand-rolled callback-after-state-update pattern. The Test Quality14. Tests have extensive boilerplate but lack edge case coverage for the Viewport The 15. Keyboard test helpers could be DRYer The SummaryThe biggest risks are the render-time DOM mutations in |
Codex ReviewOverviewThis patch adds a new Calendar API, the date and localization plumbing it depends on, and a large set of docs and demos around that surface. The overall structure is promising, but the current branch still has release-blocking gaps in day-button interaction behavior and multi-month keyboard navigation. Findings1. 🔴 [Blocking] Outside-month day buttons still perform selectionImpact: Outside-month days are exposed as disabled and styled as non-interactive, but clicking them still changes the value. That is both an accessibility mismatch and a behavior footgun: assistive tech is told the control is disabled while pointer users can still activate it. Evidence: Recommendation: Treat 2. 🔴 [Blocking] PageUp/PageDown in offset grids advances from the wrong monthImpact: The public Evidence: Recommendation: Drive month paging and looping from the body-local month for that grid, and add regression coverage for keyboard navigation in multi-month calendars. Confidence: 4/5High confidence based on a full-branch pass across the calendar/store/adapter/shared-hook changes, focused suite coverage on the touched areas, and direct repros for the two interaction bugs above. NotesFocused suites currently pass but do not cover the failing cases above:
|
Moved DOM manipulation into an effect.
Went with the recommendation to whitelist certain elements.
Refactored code to rely on precomputed
I've briefly checked this problem and it's not a quick fix.
I don't see room for improvement here. We need an update to the locale to trigger downstream re-renders.
Used a ref to avoid unnecessary re-assignments.
Updated usages in demos and tests.
Updated to use
Removed the redundant
Removed the redundant prop.
Memoized the hook return.
Replaced |
12 participants
Part of #1709
Step 1: Extract a minimal version of the Calendar component from the X repo
Preview: https://deploy-preview-1973--base-ui.netlify.app/react/components/calendar
In this PR
Adapters
The adapters expose a unified set of methods in order to support several date libraries. The MUI X Date and Time Pickers currently support 8 adapters (Day.js, Luxon, Moment, Date Fns v3+v4, Date Fns v2, Moment Jalaali, Moment Hijri and Date Fns Jalali).One of the main goal of this PR should be to decide if we want to keep this abstraction.
Decided to hide the adapter initially and use the
date-fnsadapter internally.adapter.now()method to avoid polymorphism onadapter.date())Manager / validation
The manager expose methods to interact with the value regarless of its type (date, time, date time, date range, time range and date time range).
This allow to do stuff like validate the value, set the timezone on a value, compare two values etc...
It allows to share a lot of code between the Calendar and the Range Calendar (see
useSharedCalendarRoot).On the field components, it will allow to have almost all the code agnostic of the value type and to create very light wrappers for each value type.
disableFutureanddisablePast, replaceshouldDisableDatewith aisDateUnavailablethat is not part of the validation per say like on React Aria)Calendar component
onKeyDown)eventDetailsstructureFinalization
motion/reactTemporalLocaleProviderLocalizationProviderAssert if theSetMonthpart is actually neededRemoved it, extended the public context with
setVisibleDateand added an example with month and year selectsCheck if we need to improveariaof the week header (remove aria-hidden)Follow-ups
The current approach doesn't work well for calendar when navigating quickly
trwould be rendered withrole="row"