-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix(datetime-button): fix initial text not obeying datetime constraints #31218
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
base: main
Are you sure you want to change the base?
Changes from all commits
f82ea68
ba0ebeb
7d95269
4824122
ef4c1cd
41952e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,9 +7,9 @@ import { createColorClasses } from '@utils/theme'; | |||||||||||
| import { getIonMode } from '../../global/ionic-global'; | ||||||||||||
| import type { Color } from '../../interface'; | ||||||||||||
| import type { DatetimePresentation } from '../datetime/datetime-interface'; | ||||||||||||
| import { getToday } from '../datetime/utils/data'; | ||||||||||||
| import { getLocalizedDateTime, getLocalizedTime } from '../datetime/utils/format'; | ||||||||||||
| import { getHourCycle } from '../datetime/utils/helpers'; | ||||||||||||
| import { convertDataToISO } from '../datetime/utils/manipulation'; | ||||||||||||
| import { parseDate } from '../datetime/utils/parse'; | ||||||||||||
| /** | ||||||||||||
| * @virtualProp {"ios" | "md"} mode - The mode determines which platform styles to use. | ||||||||||||
|
|
@@ -125,7 +125,7 @@ export class DatetimeButton implements ComponentInterface { | |||||||||||
| overlayEl.classList.add('ion-datetime-button-overlay'); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| componentOnReady(datetimeEl, () => { | ||||||||||||
| componentOnReady(datetimeEl, async () => { | ||||||||||||
| const datetimePresentation = (this.datetimePresentation = datetimeEl.presentation || 'date-time'); | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
|
|
@@ -138,7 +138,7 @@ export class DatetimeButton implements ComponentInterface { | |||||||||||
| * to re-render the displayed | ||||||||||||
| * text in the buttons. | ||||||||||||
| */ | ||||||||||||
| this.setDateTimeText(); | ||||||||||||
| await this.setDateTimeText(); | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this change necessary? |
||||||||||||
| addEventListener(datetimeEl, 'ionValueChange', this.setDateTimeText); | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
|
|
@@ -189,7 +189,7 @@ export class DatetimeButton implements ComponentInterface { | |||||||||||
| * ion-datetime and then format it according | ||||||||||||
| * to the locale specified on ion-datetime. | ||||||||||||
| */ | ||||||||||||
| private setDateTimeText = () => { | ||||||||||||
| private setDateTimeText = async () => { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this change necessary? |
||||||||||||
| const { datetimeEl, datetimePresentation } = this; | ||||||||||||
|
|
||||||||||||
| if (!datetimeEl) { | ||||||||||||
|
|
@@ -201,10 +201,14 @@ export class DatetimeButton implements ComponentInterface { | |||||||||||
| const parsedValues = this.getParsedDateValues(value); | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Both ion-datetime and ion-datetime-button default | ||||||||||||
| * to today's date and time if no value is set. | ||||||||||||
| * Both ion-datetime and ion-datetime-button default to today's date and | ||||||||||||
| * time if no value is set. We read the datetime's computed default so the | ||||||||||||
| * button respects the same constraints (min, max, minuteValues, etc.) that | ||||||||||||
| * the datetime applies to its own fallback, instead of using a raw "now". | ||||||||||||
| */ | ||||||||||||
| const parsedDatetimes = parseDate(parsedValues.length > 0 ? parsedValues : [getToday()]); | ||||||||||||
| const parsedDatetimes = parseDate( | ||||||||||||
| parsedValues.length > 0 ? parsedValues : [convertDataToISO(await datetimeEl.getDefaultPart())] | ||||||||||||
| ); | ||||||||||||
|
Comment on lines
+209
to
+211
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Unless there's a reason of why you went with this approach? |
||||||||||||
|
|
||||||||||||
| if (!parsedDatetimes) { | ||||||||||||
| return; | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -344,4 +344,154 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { | |||||
| await expect(page.locator('ion-datetime-button')).toContainText('Thu, November 02 01:22 AM'); | ||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| test.describe(title('datetime-button: datetime constraints'), () => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional / nice-to-have, take it or leave it. These four tests repeat the same clock setup and formatter definitions. We already share per-describe setup with test.describe(title('datetime-button: datetime constraints'), () => {
const fixedTime = new Date('2026-06-18T17:54:54.518Z');
const dateFormat = new Intl.DateTimeFormat('en-US', {
weekday: 'short',
month: 'long',
day: '2-digit',
});
const timeFormat = new Intl.DateTimeFormat('en-US', {
hour: '2-digit',
minute: '2-digit',
});
test.beforeEach(async ({ page }) => {
await page.clock.setFixedTime(fixedTime);
});Then each test drops its local const expectedTime = new Date(fixedTime);
expectedTime.setMinutes(0);.One caveat if you do take this: once |
||||||
| test('should default to exact current time with no constraints', async ({ page }) => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These constraint tests should be annotated with the issue they're covering, like the existing #27797 tests in this file do. |
||||||
| const fixedTime = new Date('2026-06-18T17:54:54.518Z'); | ||||||
| await page.clock.setFixedTime(fixedTime); | ||||||
|
|
||||||
| await page.setContent( | ||||||
| ` | ||||||
| <ion-datetime-button datetime="datetime"></ion-datetime-button> | ||||||
| <ion-datetime id="datetime" presentation="date-time" locale="en-US"></ion-datetime> | ||||||
| <script> | ||||||
| const datetime = document.querySelector('ion-datetime'); | ||||||
| datetime.formatOptions = { | ||||||
| date: { | ||||||
| weekday: "short", | ||||||
| month: "long", | ||||||
| day: "2-digit" | ||||||
| }, | ||||||
| time: { | ||||||
| hour: "2-digit", | ||||||
| minute: "2-digit" | ||||||
| } | ||||||
| } | ||||||
| </script> | ||||||
| `, | ||||||
| config | ||||||
| ); | ||||||
| await page.locator('.datetime-ready').waitFor(); | ||||||
|
|
||||||
| const dateFormat = new Intl.DateTimeFormat('en-US', { | ||||||
| weekday: 'short', | ||||||
| month: 'long', | ||||||
| day: '2-digit', | ||||||
| }); | ||||||
| const timeFormat = new Intl.DateTimeFormat('en-US', { | ||||||
| hour: '2-digit', | ||||||
| minute: '2-digit', | ||||||
| }); | ||||||
|
|
||||||
| await expect(page.locator('#date-button')).toContainText(dateFormat.format(fixedTime)); | ||||||
| await expect(page.locator('#time-button')).toContainText(timeFormat.format(fixedTime)); | ||||||
| }); | ||||||
|
|
||||||
| test('should obey minuteValues constraint', async ({ page }) => { | ||||||
| const fixedTime = new Date('2026-06-18T17:54:54.518Z'); | ||||||
| await page.clock.setFixedTime(fixedTime); | ||||||
|
|
||||||
| await page.setContent( | ||||||
| ` | ||||||
| <ion-datetime-button datetime="datetime"></ion-datetime-button> | ||||||
| <ion-datetime id="datetime" presentation="time" locale="en-US" minute-values="0"></ion-datetime> | ||||||
| <script> | ||||||
| const datetime = document.querySelector('ion-datetime'); | ||||||
| datetime.formatOptions = { | ||||||
| time: { | ||||||
| hour: "2-digit", | ||||||
| minute: "2-digit" | ||||||
| } | ||||||
| } | ||||||
| </script> | ||||||
| `, | ||||||
| config | ||||||
| ); | ||||||
| await page.locator('.datetime-ready').waitFor(); | ||||||
|
|
||||||
| await page.pause(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove the await |
||||||
|
|
||||||
| const expectedTime = fixedTime; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| expectedTime.setMinutes(0); | ||||||
|
|
||||||
| const timeFormat = new Intl.DateTimeFormat('en-US', { | ||||||
| hour: '2-digit', | ||||||
| minute: '2-digit', | ||||||
| }); | ||||||
|
|
||||||
| await expect(page.locator('#time-button')).toContainText(timeFormat.format(expectedTime)); | ||||||
| }); | ||||||
|
|
||||||
| test('should obey hourValues constraint', async ({ page }) => { | ||||||
| const fixedTime = new Date('2026-06-18T17:54:54.518Z'); | ||||||
| await page.clock.setFixedTime(fixedTime); | ||||||
|
|
||||||
| await page.setContent( | ||||||
| ` | ||||||
| <ion-datetime-button datetime="datetime"></ion-datetime-button> | ||||||
| <ion-datetime id="datetime" presentation="time" locale="en-US" hour-values="0"></ion-datetime> | ||||||
| <script> | ||||||
| const datetime = document.querySelector('ion-datetime'); | ||||||
| datetime.formatOptions = { | ||||||
| time: { | ||||||
| hour: "2-digit", | ||||||
| minute: "2-digit" | ||||||
| } | ||||||
| } | ||||||
| </script> | ||||||
| `, | ||||||
| config | ||||||
| ); | ||||||
| await page.locator('.datetime-ready').waitFor(); | ||||||
|
|
||||||
| await page.pause(); | ||||||
|
|
||||||
| const expectedTime = fixedTime; | ||||||
| expectedTime.setHours(0); | ||||||
|
|
||||||
| const timeFormat = new Intl.DateTimeFormat('en-US', { | ||||||
| hour: '2-digit', | ||||||
| minute: '2-digit', | ||||||
| }); | ||||||
|
|
||||||
| await expect(page.locator('#time-button')).toContainText(timeFormat.format(expectedTime)); | ||||||
| }); | ||||||
|
|
||||||
| test('should obey monthValues constraint', async ({ page }) => { | ||||||
| const fixedTime = new Date('2026-06-18T17:54:54.518Z'); | ||||||
| await page.clock.setFixedTime(fixedTime); | ||||||
|
|
||||||
| await page.setContent( | ||||||
| ` | ||||||
| <ion-datetime-button datetime="datetime"></ion-datetime-button> | ||||||
| <ion-datetime id="datetime" presentation="date" locale="en-US" month-values="1"></ion-datetime> | ||||||
| <script> | ||||||
| const datetime = document.querySelector('ion-datetime'); | ||||||
| datetime.formatOptions = { | ||||||
| date: { | ||||||
| weekday: "short", | ||||||
| month: "long", | ||||||
| day: "2-digit" | ||||||
| } | ||||||
| } | ||||||
| </script> | ||||||
| `, | ||||||
| config | ||||||
| ); | ||||||
| await page.locator('.datetime-ready').waitFor(); | ||||||
|
|
||||||
| await page.pause(); | ||||||
|
|
||||||
| const expectedTime = fixedTime; | ||||||
| expectedTime.setMonth(0); | ||||||
|
|
||||||
| const dateFormat = new Intl.DateTimeFormat('en-US', { | ||||||
| weekday: 'short', | ||||||
| month: 'long', | ||||||
| day: '2-digit', | ||||||
| }); | ||||||
|
|
||||||
| await expect(page.locator('#date-button')).toContainText(dateFormat.format(expectedTime)); | ||||||
| }); | ||||||
| }); | ||||||
| }); | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -604,6 +604,40 @@ export class Datetime implements ComponentInterface { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Get the closest valid DatetimeParts according to the restrictions on this Datetime | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param parts The DatetimeParts to find the closest valid value for | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private getClosestDatetimeParts(parts: DatetimeParts) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This helper can go away. It only exists because the earlier |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const hourValues = (this.parsedHourValues = convertToArrayOfNumbers(this.hourValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const minuteValues = (this.parsedMinuteValues = convertToArrayOfNumbers(this.minuteValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const monthValues = (this.parsedMonthValues = convertToArrayOfNumbers(this.monthValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const yearValues = (this.parsedYearValues = convertToArrayOfNumbers(this.yearValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const dayValues = (this.parsedDayValues = convertToArrayOfNumbers(this.dayValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return getClosestValidDate({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| refParts: parts, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monthValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dayValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| yearValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hourValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| minuteValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| minParts: this.minParts, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| maxParts: this.maxParts, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Returns the default parts the datetime falls back to when no value is set: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * today's date and time snapped to the closest value allowed by the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * component's constraints (`min`, `max`, and the `*Values` props). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @internal | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Method() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async getDefaultPart(): Promise<DatetimeParts> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return this.defaultParts; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+629
to
+639
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend an
Making it
Suggested change
The /**
* Both ion-datetime and ion-datetime-button default to today's date and
* time if no value is set. We read the datetime's computed default so the
* button respects the same constraints (min, max, minuteValues, etc.) that
* the datetime applies to its own fallback, instead of using a raw "now".
*/
const parsedDatetimes =
parsedValues.length > 0 ? parseDate(parsedValues) : [await datetimeEl.getDefaultPart()]; |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private warnIfIncorrectValueUsage = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { multiple, value } = this; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!multiple && Array.isArray(value)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1495,27 +1529,12 @@ export class Datetime implements ComponentInterface { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| warnIfTimeZoneProvided(el, formatOptions); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const hourValues = (this.parsedHourValues = convertToArrayOfNumbers(this.hourValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in https://github.com/ionic-team/ionic-framework/pull/31218/changes#r3463042864, I would revert this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const minuteValues = (this.parsedMinuteValues = convertToArrayOfNumbers(this.minuteValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const monthValues = (this.parsedMonthValues = convertToArrayOfNumbers(this.monthValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const yearValues = (this.parsedYearValues = convertToArrayOfNumbers(this.yearValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const dayValues = (this.parsedDayValues = convertToArrayOfNumbers(this.dayValues)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const todayParts = (this.todayParts = parseDate(getToday())!); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.processMinParts(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.processMaxParts(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.defaultParts = getClosestValidDate({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in https://github.com/ionic-team/ionic-framework/pull/31218/changes#r3463042864, I would revert this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| refParts: todayParts, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| monthValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dayValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| yearValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hourValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| minuteValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| minParts: this.minParts, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| maxParts: this.maxParts, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.defaultParts = this.getClosestDatetimeParts(todayParts); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.processValue(this.value); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Why is this change necessary?