Conversation
…month-list-disable-offset-EIX-314
Luxon has two numbering systems that are each internally correct: ISO ordinals (`DateTime.month` 1-12, `DateTime.weekday` 1-7 Monday=1) and plain array offsets (`Info.months()` / `Info.weekdays()` are 0-based). Bugs appear wherever one is compared against or used to index the other. `calendar-units.ts` had drawn the boundary in index-space, which is the losing side: DateTime touchpoints far outnumber array touchpoints, so every helper was another chance to convert in the wrong direction. Invert it. A calendar position is now carried as a `DateTime` and never as a bare unit number, and names are derived by formatting that DateTime rather than by indexing a name array. An array index no longer escapes a single `.map()`, so neither base is written down outside the util. - `calendar-units.ts` becomes a thin calendar vocabulary over DateTime (`monthsOfYear`, `monthNameOf`, `dayOfMonth`, `weekdayNamesFrom`, `weekdayColumnOf`, the `is*WithinRange` predicates). Every index-conversion helper is gone. - `date-picker.tsx` carries `selectedMonthDate` / `tempMonthDate` as DateTime rather than as four separate year/month numbers. - The `isWithinMinMaxMonth` off-by-one is fixed structurally, not patched: because a month now carries its own year, the ordinal-vs-offset mismatch is not expressible. - Drop the dead `_minDateObj` / `_maxDateObj` fields. They were assigned via the locale-aware `parseWithLocale` but never read, while the live path re-parsed through the locale-blind `DateTime.fromFormat` on every `getDayClasses` call (~84 parses per render). This is a deliberate behaviour change: min/max bounds now honour the locale. - Retarget the `calendarUnitRules` ESLint block. It named helpers that no longer exist, and `no-restricted-syntax` is type-blind so it cannot enforce transport anyway - the signatures do that. Lint now guards the step before: the ways a bare unit number gets manufactured, including a new ban on `Info.months()` / `Info.weekdays()`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughChangesCalendar unit migration
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The calendar migration preserves the inspected date-range behavior, with no concrete merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
This is a user-facing month min/max bug fix, so it needs a changeset for @siemens/ix (patch) or it will not ship in release notes. |
| @@ -428,12 +454,10 @@ export class DatePicker | |||
| */ | |||
| @Method() | |||
| async updateSelectedYearMonth(date: DateTime) { | |||
There was a problem hiding this comment.
renderMonths() builds the list from tempMonthDate.year, but updateSelectedYearMonth only assigns selectedMonthDate.
from/to watchers and ix-date-dropdown (re-select same range) call this method, so the calendar can show year A while the month dropdown still lists/disables year B.
Please also set this.tempMonthDate = this.selectedMonthDate here, same as selectMonth / changeCalendarView.
| 'enables the month a single-month range sits in', | ||
| async ({ mount, page }) => { | ||
| await mount( | ||
| `<ix-date-picker from="2026/07/06" min-date="2026/07/05" max-date="2026/07/15" single-selection></ix-date-picker>` |
There was a problem hiding this comment.
These asserts use English month names (July, June, …) but the mounts do not set locale="en". The translation test in this file already pins locale="de", so this will fail on a non-English runner.
Please add locale="en" to these mounts. A case matching #2780 (min-date="2026/03/31" / max-date="2027/01/31") would also help.
|
The PR title ( Please rename the pull request to something like Also please rebase onto latest |
| @@ -428,12 +454,10 @@ export class DatePicker | |||
| */ | |||
| @Method() | |||
There was a problem hiding this comment.
The DateTime model is good! however the displayed month is still two clocks (selectedMonthDate vs tempMonthDate) written four different ways.
renderMonths() uses tempMonthDate.year, while the header and grid use selectedMonthDate. updateSelectedYearMonth (from/to watchers and ix-date-dropdown) only sets selectedMonthDate, so the calendar can show year A while the month list still disables year B. The JSDoc says tempMonthDate diverges only while picking a year before a month, but year click already commits both.
Pls consider to route all four paths through one setter, e.g. setDisplayedMonth(month), that assigns both to month.startOf('month'). Keep the current UX (year click updates the calendar immediately). Then the tempMonthDate comment should match that, or go away.
There was a problem hiding this comment.
It is related to the next comment. Pls consider both as one suggestion/finding
|
|
||
| import { DateTime, Info } from 'luxon'; | ||
|
|
||
| /** |
There was a problem hiding this comment.
A lot of the new commentary restates the 0- vs 1-based rule (file essay, almost every helper, ESLint block, and tempMonthDate / weekStart in date-picker.tsx). That is heavier than nearby core utils (date-time-locale.ts is a short JSDoc only when the contract is not obvious).
Please keep public @Prop/@Method docs, one short module note, and the ESLint message strings. Trim the rest — including the tempMonthDate JSDoc if we keep a single setDisplayedMonth (it currently describes a year-then-month flow that year click does not do). Also the locale prop still says locale sets weekday order; weekStartIndex does.
|
Please consider to add |
| allConfig: js.configs.all, | ||
| }); | ||
|
|
||
| // `no-restricted-syntax` is not additive across flat-config objects - a later |
There was a problem hiding this comment.
Reading eslint.config.cjs: this PR extracts the existing ! / definite-assignment no-restricted-syntax rules so they can be re-spread, then adds calendarUnitRules on the date-* components (ban .month, .weekday, Info.months() / Info.weekdays(), new Date(y, m, …); tests and calendar-units.ts excluded).
That is a guardrail so we do not mix Luxon 1-based ordinals with 0-based name arrays again. What I see it is not required for the month min/max fix — calendar-units + the picker DateTime state already do that. Flat config replaces no-restricted-syntax rather than merging, which is why the ! rules had to be extracted.
CI should not fail if this file is reverted on this PR: core lint is still the main ! rules, and the new picker code does not trip them.
Please move the ESLint change to a follow-up/new PR, so this PR stays the #2780 / EIX-314 behaviour fix.
2 participants
💡 What is the current behavior?
There are numerous bugs in the code related to mismatched indexes being compared across the logic. E.g. the month disabling code that compares 0-base index month names to 1-base month list.
GitHub Issue Number: #2780
🆕 What is the new behavior?
This PR fixes any existing Date index mismatches and removes the scope for any further mismatches via normalising all DateTime logic to use consistent Luxon objects and then resolve strings and values directly from these objects when required, rather than using the strings and values for comparison.
Tests have been added to ensure behaviour is kept consistent, and to cover the revealed prior bugs.
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit
New Features
Bug Fixes
Tests