Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
ce4c2eb
always jump to selected month when value changes
Jul 14, 2023
39d6871
remove remaining activePartsClone refs
Jul 14, 2023
5c13d59
add ability to animate smoothly to month of new value (needs cleanup …
Jul 17, 2023
e6a9ed5
remove DatetimeMonth interface and include day when forcing month
Jul 18, 2023
5ae63ec
rename state variable to use date instead of month
Jul 18, 2023
27d76c7
remove missed instance of manually getting forced day
Jul 18, 2023
237f015
nevermind this totally works, was jumping to another year lol
Jul 18, 2023
e79a558
actually wait for next/prevMonth call to finish before unsetting forc…
Jul 18, 2023
59e0bde
remove console logs
Jul 18, 2023
db126be
check if we actually scrolled to the forced month before returning it
Jul 18, 2023
2877537
pull grid style check into private getter
Jul 18, 2023
21e3648
remove unused variables
Jul 18, 2023
f90bcc4
don't animate if in closed datetime-button
Jul 18, 2023
79b8e92
switch lock promise to a local variable
Jul 18, 2023
69267e4
more comments
Jul 18, 2023
ab9bf27
lint
Jul 18, 2023
cbfc8e9
add tests
Jul 19, 2023
4cf6270
Merge branch 'feature-7.3' into FW-3846
averyjohnston Jul 19, 2023
fd9508e
await processValue in places it was already being called
Jul 24, 2023
10b70ed
tweak variable name to match language
Jul 24, 2023
dda4f0b
clean up promise typing
Jul 24, 2023
7a3f927
don't animate if month/year picker is open
Jul 24, 2023
3dd1732
fix error when setting value to improperly formatted string
Jul 24, 2023
607f43c
handle improper date formatting for multiple values
Jul 24, 2023
c57c6fe
update parseDate type sigs to include returning undefined when val co…
Jul 24, 2023
f3679c1
fix min/max parsing erroring out if year isn't provided
Jul 24, 2023
3fe32dc
lint
Jul 24, 2023
53e288e
avoid crash when manually setting value to empty array
Jul 24, 2023
692a270
fix month scroll breaking when value is changed with time popover open
Jul 25, 2023
17224a1
use undefined instead of null for forceRenderDate
Aug 1, 2023
a077005
re-use current date in both branches of generateMonths
Aug 1, 2023
bf67e71
remove animate param from processValue
Aug 1, 2023
07426c6
set forceRenderDate to whole targetValue for consistency
Aug 1, 2023
d2cd559
also match isBefore language in generateMonths
Aug 1, 2023
aa9d1da
lint
Aug 1, 2023
24061b6
Merge branch 'feature-7.3' into FW-3846
averyjohnston Aug 1, 2023
cf4128f
try adding animate param back in but also animating when calling reset
Aug 1, 2023
5c842cf
check if calendar body is ready before animating; take animate param …
Aug 3, 2023
90a92d1
always assume we've scrolled to the forced date if it's set
Aug 4, 2023
bf0fc24
fix forceRenderDate comment
Aug 4, 2023
50703df
pull scroll waiting stuff into separate method so processValue can be…
Aug 4, 2023
8936c02
lint
Aug 4, 2023
cb3f835
Merge remote-tracking branch 'origin/feature-7.3' into FW-3846
Aug 4, 2023
b1ccb75
remove unneeded manual month/year selection from test
Aug 4, 2023
fbf2f9c
don't animate if target month and/or year are undefined
Aug 8, 2023
b85c7df
combine didChangeMonth and monthYearDefined conditions
Aug 8, 2023
3d79ec9
add isGridStyle to destructure from `this`
Aug 21, 2023
419b12a
Merge branch 'feature-7.4' into FW-3846
Aug 21, 2023
129b370
lint
Aug 21, 2023
d884163
Merge branch 'feature-7.4' into FW-3846
averyjohnston Aug 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 112 additions & 87 deletions core/src/components/datetime/datetime.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,7 @@ export class Datetime implements ComponentInterface {

private prevPresentation: string | null = null;

/**
* Duplicate reference to `activeParts` that does not trigger a re-render of the component.
* Allows caching an instance of the `activeParts` in between render cycles.
*/
private activePartsClone: DatetimeParts | DatetimeParts[] = [];
private resolveForceDateScrolling?: () => void;

@State() showMonthAndYear = false;

Expand All @@ -141,6 +137,16 @@ export class Datetime implements ComponentInterface {
@State() isPresented = false;
@State() isTimePopoverOpen = false;

/**
* When non-null, will force the datetime to render the month
* containing the specified date. This enables animating the
* transition to a new value, and should be reset to null once
* the transition is finished and the forced month is now in view.
*
* Applies to grid-style datetimes only.
*/
@State() forceRenderDate: DatetimeParts | null = null;

/**
* The color to use from your application's color palette.
* Default options are: `"primary"`, `"secondary"`, `"tertiary"`, `"success"`, `"warning"`, `"danger"`, `"light"`, `"medium"`, and `"dark"`.
Expand Down Expand Up @@ -222,6 +228,12 @@ export class Datetime implements ComponentInterface {
*/
@Prop() presentation: DatetimePresentation = 'date-time';

private get isGridStyle() {
const { presentation, preferWheel } = this;
const hasDatePresentation = presentation === 'date' || presentation === 'date-time' || presentation === 'time-date';
return hasDatePresentation && !preferWheel;
}

/**
* The text to display on the picker's cancel button.
*/
Expand Down Expand Up @@ -303,11 +315,6 @@ export class Datetime implements ComponentInterface {
this.parsedMinuteValues = convertToArrayOfNumbers(this.minuteValues);
}

@Watch('activeParts')
protected activePartsChanged() {
this.activePartsClone = this.activeParts;
}

/**
* The locale to use for `ion-datetime`. This
* impacts month and day name formatting.
Expand Down Expand Up @@ -358,53 +365,10 @@ export class Datetime implements ComponentInterface {
*/
@Watch('value')
protected valueChanged() {
const { value, minParts, maxParts, workingParts } = this;
const { value } = this;

if (this.hasValue()) {
this.warnIfIncorrectValueUsage();

/**
* Clones the value of the `activeParts` to the private clone, to update
* the date display on the current render cycle without causing another render.
*
* This allows us to update the current value's date/time display without
* refocusing or shifting the user's display (leaves the user in place).
*/
const valueDateParts = parseDate(value);
if (valueDateParts) {
warnIfValueOutOfBounds(valueDateParts, minParts, maxParts);

if (Array.isArray(valueDateParts)) {
this.activePartsClone = [...valueDateParts];
} else {
const { month, day, year, hour, minute } = valueDateParts;
const ampm = hour != null ? (hour >= 12 ? 'pm' : 'am') : undefined;

this.activePartsClone = {
...this.activeParts,
month,
day,
year,
hour,
minute,
ampm,
};

/**
* The working parts am/pm value must be updated when the value changes, to
* ensure the time picker hour column values are generated correctly.
*
* Note that we don't need to do this if valueDateParts is an array, since
* multiple="true" does not apply to time pickers.
*/
this.setWorkingParts({
...workingParts,
ampm,
});
}
} else {
printIonWarning(`Unable to parse date string: ${value}. Please provide a valid ISO 8601 datetime string.`);
}
this.processValue(value, true);
}

this.emitStyle();
Expand Down Expand Up @@ -597,18 +561,18 @@ export class Datetime implements ComponentInterface {
* data. This should be used when rendering an
* interface in an environment where the `value`
* may not be set. This function works
* by returning the first selected date in
* "activePartsClone" and then falling back to
* defaultParts if no active date is selected.
* by returning the first selected date and then
* falling back to defaultParts if no active date
* is selected.
*/
private getActivePartsWithFallback = () => {
const { defaultParts } = this;
return this.getActivePart() ?? defaultParts;
};

private getActivePart = () => {
const { activePartsClone } = this;
return Array.isArray(activePartsClone) ? activePartsClone[0] : activePartsClone;
const { activeParts } = this;
return Array.isArray(activeParts) ? activeParts[0] : activeParts;
};

private closeParentOverlay = () => {
Expand All @@ -628,7 +592,7 @@ export class Datetime implements ComponentInterface {
};

private setActiveParts = (parts: DatetimeParts, removeDate = false) => {
const { multiple, minParts, maxParts, activePartsClone } = this;
const { multiple, minParts, maxParts, activeParts } = this;

/**
* When setting the active parts, it is possible
Expand All @@ -644,16 +608,7 @@ export class Datetime implements ComponentInterface {
this.setWorkingParts(validatedParts);

if (multiple) {
/**
* We read from activePartsClone here because valueChanged() only updates that,
* so it's the more reliable source of truth. If we read from activeParts, then
* if you click July 1, manually set the value to July 2, and then click July 3,
* the new value would be [July 1, July 3], ignoring the value set.
*
* We can then pass the new value to activeParts (rather than activePartsClone)
* since the clone will be updated automatically by activePartsChanged().
*/
const activePartsArray = Array.isArray(activePartsClone) ? activePartsClone : [activePartsClone];
const activePartsArray = Array.isArray(activeParts) ? activeParts : [activeParts];
if (removeDate) {
this.activeParts = activePartsArray.filter((p) => !isSameDay(p, validatedParts));
} else {
Expand Down Expand Up @@ -912,6 +867,29 @@ export class Datetime implements ComponentInterface {
const monthBox = month.getBoundingClientRect();
if (Math.abs(monthBox.x - box.x) > 2) return;

/**
* If we're force-rendering a month, and we've scrolled to
* that month, return that.
*
* Checking that we've actually scrolled to the forced month
* is mostly for safety; in theory, if there's a forced month,
* that means a new value was manually set, so we should have
* automatically animated directly to it.
*/
const { forceRenderDate } = this;
const firstDayEl = month.querySelector('.calendar-day');
const dataMonth = firstDayEl?.getAttribute('data-month');
const dataYear = firstDayEl?.getAttribute('data-year');
if (
forceRenderDate !== null &&
dataMonth &&
dataYear &&
parseInt(dataMonth) === forceRenderDate.month &&
parseInt(dataYear) === forceRenderDate.year
) {
return { month: forceRenderDate.month, year: forceRenderDate.year, day: forceRenderDate.day };
}

/**
* From here, we can determine if the start
* month or the end month was scrolled into view.
Expand Down Expand Up @@ -980,6 +958,10 @@ export class Datetime implements ComponentInterface {

calendarBodyRef.scrollLeft = workingMonth.clientWidth * (isRTL(this.el) ? -1 : 1);
calendarBodyRef.style.removeProperty('overflow');

if (this.resolveForceDateScrolling) {
this.resolveForceDateScrolling();
}
});
};

Expand Down Expand Up @@ -1196,11 +1178,11 @@ export class Datetime implements ComponentInterface {
});
}

private processValue = (value?: string | string[] | null) => {
private processValue = async (value?: string | string[] | null, animate = false) => {
const hasValue = value !== null && value !== undefined;
const valueToProcess = hasValue ? parseDate(value) : this.defaultParts;

const { minParts, maxParts } = this;
const { minParts, maxParts, workingParts, el } = this;

this.warnIfIncorrectValueUsage();

Expand All @@ -1222,19 +1204,11 @@ export class Datetime implements ComponentInterface {
* that the values don't necessarily have to be in order.
*/
const singleValue = Array.isArray(valueToProcess) ? valueToProcess[0] : valueToProcess;
const targetValue = clampDate(singleValue, minParts, maxParts);

const { month, day, year, hour, minute } = clampDate(singleValue, minParts, maxParts);
const { month, day, year, hour, minute } = targetValue;
const ampm = parseAmPm(hour!);

this.setWorkingParts({
month,
day,
year,
hour,
minute,
ampm,
});

/**
* Since `activeParts` indicates a value that
* been explicitly selected either by the
Expand Down Expand Up @@ -1262,6 +1236,58 @@ export class Datetime implements ComponentInterface {
*/
this.activeParts = [];
}

/**
* Only animate if:
* 1. We're using grid style (wheel style pickers should just jump to new value)
* 2. The month and/or year actually changed (otherwise there's nothing to animate to)
* 3. The datetime is visible (prevents animation when in collapsed datetime-button, for example)
*/
const didChangeMonth = month !== workingParts.month || year !== workingParts.year;
const elIsVisible = el.offsetParent !== null;
if (animate && this.isGridStyle && didChangeMonth && elIsVisible) {
/**
* Tell other render functions that we need to force the
* target month to appear in place of the actual next/prev month.
* Because this is a State variable, a rerender will be triggered
* automatically, updating the rendered months.
*/
this.forceRenderDate = { month, year, day };

/**
* Flag that we've started scrolling to the forced date.
* The resolve function will be called by the datetime's
* scroll listener when it's done updating everything.
* This is a replacement for making prev/nextMonth async,
* since the logic we're waiting on is in a listener.
*/
const forceDateScrollingPromise: Promise<void> = new Promise((resolve) => {
this.resolveForceDateScrolling = resolve;
});

/**
* Animate smoothly to the forced month. This will also update
* workingParts and correct the surrounding months for us.
*/
const targetMonthIsEarlier = isBefore(targetValue, workingParts);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Threading:

My understanding of the original issue is that asynchronously setting the datetime on load does not update the view. So if the datetime is going to be set either on load or shortly after load, will users even see this scrolling animation? For example, if a datetime in a modal has its value set as the modal opens, the scrolling would either happen on screen or as the modal transitions in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liamdebeasi Before I get too deep in bug fixes here, I wanted to check whether you still had any concerns with moving forward with the animation. I can understand wanting to avoid making datetime even more complex, and deciding to scrap the animation part would make fixing things up here easier, but there are a few reasons why I still feel we should go through with it:

  1. The animation matches native behavior, as mentioned above.
  2. The datetime won't always be hidden when the value is updated async. While that will definitely be our recommendation (see docs(datetime): add best practices for setting value async ionic-docs#3053), we should avoid glitchy-looking behavior when it isn't the dev's desired pattern. For example, an app could have a dropdown with appointments, and the datetime's value updates to reflect which one you've selected.
  3. The animation is something we already decided on in the design doc, and I would rather commit to that than constantly be rethinking our decisions. This isn't a case where we missed something that dramatically changes the scope; we already recognized that the animation will be difficult, and decided to proceed anyway.

Copy link
Contributor

@liamdebeasi liamdebeasi Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we shouldn't omit the animation just because it is challenging. But I'd also like us to keep in mind that the primary objective is to update the UI, with the animation being a secondary goal. This behavior is similar to #23560 where it is closer to the spec, but it's not critical functionality.

I'm not advocating for removing this, but I do want to make sure we are being cautious. If you recall the old IntersectionObserver implementation in datetime, that consumed a lot of our time before we eventually abandoned that approach in favor of a scroll listener.

targetMonthIsEarlier ? this.prevMonth() : this.nextMonth();
await forceDateScrollingPromise;
this.resolveForceDateScrolling = undefined;
this.forceRenderDate = null;
} else {
/**
* We only need to do this if we didn't just animate to a new month,
* since prevMonth/nextMonth call setWorkingParts for us.
*/
this.setWorkingParts({
month,
day,
year,
hour,
minute,
ampm,
});
}
};

componentWillLoad() {
Expand Down Expand Up @@ -2046,7 +2072,7 @@ export class Datetime implements ComponentInterface {
const { isActive, isToday, ariaLabel, ariaSelected, disabled, text } = getCalendarDayState(
this.locale,
referenceParts,
this.activePartsClone,
this.activeParts,
this.todayParts,
this.minParts,
this.maxParts,
Expand Down Expand Up @@ -2155,7 +2181,7 @@ export class Datetime implements ComponentInterface {
private renderCalendarBody() {
return (
<div class="calendar-body ion-focusable" ref={(el) => (this.calendarBodyRef = el)} tabindex="0">
{generateMonths(this.workingParts).map(({ month, year }) => {
{generateMonths(this.workingParts, this.forceRenderDate).map(({ month, year }) => {
return this.renderMonth(month, year);
})}
</div>
Expand Down Expand Up @@ -2384,7 +2410,6 @@ export class Datetime implements ComponentInterface {
const monthYearPickerOpen = showMonthAndYear && !isMonthAndYearPresentation;
const hasDatePresentation = presentation === 'date' || presentation === 'date-time' || presentation === 'time-date';
const hasWheelVariant = hasDatePresentation && preferWheel;
const hasGrid = hasDatePresentation && !preferWheel;

renderHiddenInput(true, el, name, formatValue(value), disabled);

Expand All @@ -2404,7 +2429,7 @@ export class Datetime implements ComponentInterface {
[`datetime-presentation-${presentation}`]: true,
[`datetime-size-${size}`]: true,
[`datetime-prefer-wheel`]: hasWheelVariant,
[`datetime-grid`]: hasGrid,
[`datetime-grid`]: this.isGridStyle,
}),
}}
>
Expand Down
24 changes: 24 additions & 0 deletions core/src/components/datetime/test/prefer-wheel/datetime.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,30 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => {

await ionChange.next();
});

test('should jump to selected date when programmatically updating value', async ({ page }) => {
await page.setContent(
`
<ion-datetime presentation="date" prefer-wheel="true" min="2019-05-05" max="2023-10-01" value="2019-05-30"></ion-datetime>
`,
config
);

await page.waitForSelector('.datetime-ready');
const datetime = page.locator('ion-datetime');

await datetime.evaluate((el: HTMLIonDatetimeElement) => (el.value = '2021-05-25T12:40:00.000Z'));
await page.waitForChanges();

const selectedMonth = datetime.locator('.month-column .picker-item-active');
const selectedDay = datetime.locator('.day-column .picker-item-active');
const selectedYear = datetime.locator('.year-column .picker-item-active');

await expect(selectedMonth).toHaveText(/May/);
await expect(selectedDay).toHaveText(/25/);
await expect(selectedYear).toHaveText(/2021/);
});

test.describe('datetime: date wheel localization', () => {
test('should correctly localize the date data', async ({ page }) => {
await page.setContent(
Expand Down
14 changes: 14 additions & 0 deletions core/src/components/datetime/test/set-value/datetime.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
const activeDate = page.locator('ion-datetime .calendar-day-active');
await expect(activeDate).toHaveText('25');
});

test('should update the active time when value is initially set', async ({ page }) => {
await page.goto('/src/components/datetime/test/set-value', config);
await page.waitForSelector('.datetime-ready');
Expand All @@ -27,6 +28,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
const activeDate = page.locator('ion-datetime .time-body');
await expect(activeDate).toHaveText('12:40 PM');
});

test('should update active item when value is not initially set', async ({ page }) => {
await page.setContent(
`
Expand Down Expand Up @@ -64,5 +66,17 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
await expect(activeDayButton).toHaveAttribute('data-month', '10');
await expect(activeDayButton).toHaveAttribute('data-year', '2021');
});

test('should scroll to new month when value is initially set and then updated', async ({ page }) => {
await page.goto('/src/components/datetime/test/set-value', config);
await page.waitForSelector('.datetime-ready');

const datetime = page.locator('ion-datetime');
await datetime.evaluate((el: HTMLIonDatetimeElement) => (el.value = '2021-05-25T12:40:00.000Z'));
await page.waitForChanges();

const calendarHeader = datetime.locator('.calendar-month-year');
await expect(calendarHeader).toHaveText(/May 2021/);
});
});
});
Loading