Skip to content

feat: add showMultiDayTimes property to calendar component#2204

Open
iobuhov wants to merge 3 commits intomainfrom
feat/showMultiDayTimes_Property-migrated
Open

feat: add showMultiDayTimes property to calendar component#2204
iobuhov wants to merge 3 commits intomainfrom
feat/showMultiDayTimes_Property-migrated

Conversation

@iobuhov
Copy link
Copy Markdown
Collaborator

@iobuhov iobuhov commented May 7, 2026

Summary

This PR adds the showMultiDayTimes property to the calendar component.

Migrated from #2121 (cherry-picked onto fresh main branch to resolve conflicts).

Original PR

#2121

🤖 Generated with Claude Code

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed


What was reviewed

File Change
packages/pluggableWidgets/calendar-web/src/Calendar.xml New showMultiDayTimes boolean property added
packages/pluggableWidgets/calendar-web/src/helpers/CalendarPropsBuilder.ts showMultiDayTimes forwarded to calendar props; eventTimeRangeStartFormat and eventTimeRangeEndFormat added
packages/pluggableWidgets/calendar-web/src/__tests__/Calendar.spec.tsx 183 lines of new unit tests for the new property and time-range formats
packages/pluggableWidgets/calendar-web/src/__tests__/__snapshots__/Calendar.spec.tsx.snap Snapshot updated with data-show-multi-day-times
packages/pluggableWidgets/calendar-web/typings/CalendarProps.d.ts showMultiDayTimes: boolean added to both CalendarContainerProps and CalendarPreviewProps

Skipped (out of scope): dist/, pnpm-lock.yaml


Findings

🔶 Medium — Missing CHANGELOG entry

File: packages/pluggableWidgets/calendar-web/CHANGELOG.md
Problem: The [Unreleased] section is empty. This PR adds a new XML property (showMultiDayTimes), new runtime behavior (two new format handlers — eventTimeRangeStartFormat / eventTimeRangeEndFormat), and extends the showEventDate=false blanking guard. All three qualify as public API / behavior changes that require a Keep a Changelog entry.
Fix: Add to the [Unreleased] section, e.g.:

## [Unreleased]

### Added

- Added `showMultiDayTimes` property that controls whether events spanning multiple days show start/end times in week and day views instead of appearing in the all-day row.

Run pnpm -w changelog to scaffold the entry if needed.


⚠️ Low — Modifying a generated file directly

File: packages/pluggableWidgets/calendar-web/typings/CalendarProps.d.ts lines 92, 148
Problem: The file header clearly states:

* This file was generated from Calendar.xml
* WARNING: All changes made to this file will be overwritten

AGENTS.md also says: "Never modify dist/, generated files, or lockfiles". The manual addition of showMultiDayTimes: boolean is correct content, but the next time the toolchain regenerates typings from the XML it will produce the same result — making the manual edit either harmless or overwritten. There is no risk today, but editing generated files trains a bad habit and makes diffs harder to reason about.
Note: If the CI pipeline does not regenerate typings automatically, this is safe to merge as-is, but ideally the edit should be driven by the XML change alone and validated by a build step.


Positives

  • showEventDate=false guard correctly extended to blank both eventTimeRangeStartFormat and eventTimeRangeEndFormat, not just eventTimeRangeFormat — consistent with the existing blanking logic.
  • New XML key follows lowerCamelCase convention (showMultiDayTimes) and matches the TypeScript prop name exactly.
  • Both CalendarContainerProps and CalendarPreviewProps interfaces updated in lockstep.
  • Test coverage is thorough: true/false toggle, timeFormat absent, showEventDate=false override, and format-consistency across all three range formatters — all covered as separate cases.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
samuelreichert
samuelreichert previously approved these changes May 7, 2026
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/calendar-web/src/Calendar.xml New showMultiDayTimes boolean property added
packages/pluggableWidgets/calendar-web/typings/CalendarProps.d.ts showMultiDayTimes: boolean added to CalendarContainerProps and CalendarPreviewProps
packages/pluggableWidgets/calendar-web/src/helpers/CalendarPropsBuilder.ts Pass-through + new eventTimeRangeStartFormat / eventTimeRangeEndFormat formats when timeFormat is set
packages/pluggableWidgets/calendar-web/src/__tests__/Calendar.spec.tsx 183 lines of new test coverage for showMultiDayTimes, start/end formats, and showEventDate=false override
packages/pluggableWidgets/calendar-web/src/__tests__/__snapshots__/Calendar.spec.tsx.snap Snapshot updated to include data-show-multi-day-times and serialise min/max as ISO strings
packages/pluggableWidgets/calendar-web/CHANGELOG.md Unreleased entry added

Skipped (out of scope): dist/, pnpm-lock.yaml


Findings

⚠️ Low — eventTimeRangeStartFormat / eventTimeRangeEndFormat always set, regardless of showMultiDayTimes

File: packages/pluggableWidgets/calendar-web/src/helpers/CalendarPropsBuilder.ts lines 169–178
Note: The new showMultiDayTimes prop is correctly forwarded to react-big-calendar (line 89), but the eventTimeRangeStartFormat and eventTimeRangeEndFormat format functions are registered unconditionally whenever timeFormat is set — not gated on showMultiDayTimes. In react-big-calendar, these format functions are specifically called for multi-day spanning events (i.e., the start/end caps rendered when showMultiDayTimes=true). When showMultiDayTimes=false the library won't use them, so this is harmless today; but a reader expecting them to reflect the showMultiDayTimes setting will be confused. Consider a comment explaining the unconditional registration, or guard them:

if (this.props.showMultiDayTimes) {
    formats.eventTimeRangeStartFormat = ...;
    formats.eventTimeRangeEndFormat = ...;
}

⚠️ Low — Mock localizer object duplicated in four describe blocks

File: packages/pluggableWidgets/calendar-web/src/__tests__/Calendar.spec.tsx lines 265–440
Note: The mockLocalizer shape (format, parse, startOfWeek, getDay, messages) is copy-pasted four times across the new describe blocks with minor variations in the format stub. Extracting a baseMockLocalizer constant at module level and overriding format inline where needed would reduce duplication and make failures easier to trace.


Positives

  • CHANGELOG entry is in the correct ## [Unreleased] section following Keep a Changelog format.
  • XML property key showMultiDayTimes is lowerCamelCase and matches the TypeScript prop exactly.
  • showEventDate=false override correctly covers all three format keys (eventTimeRangeFormat, eventTimeRangeStartFormat, eventTimeRangeEndFormat), ensuring the existing hide-times behaviour isn't broken by the new code path — and the tests specifically verify this.
  • Snapshot was updated cleanly: min/max are now serialised as ISO strings via .toISOString() in the mock, removing the timezone-sensitive toString() output that could cause flaky CI failures on different locales.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants