Skip to content
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

fix(datetime): scroll to newly selected date when value changes #27806

Merged
merged 50 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions core/src/components/datetime-button/datetime-button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,10 @@ export class DatetimeButton implements ComponentInterface {
*/
const parsedDatetimes = parseDate(parsedValues.length > 0 ? parsedValues : [getToday()]);

if (!parsedDatetimes) {
liamdebeasi marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if this is a bug, but if I open the month/year picker as the view datetime begins to animate, the animation gets cancelled and the view never updates to the new value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually seeing it update; maybe I'm just not doing it quickly enough? (Screencast below.) Even if there is a specific timing that borks it, though, I'm not too worried as long as the highlighted date is correct once you do scroll to it. Seems like a small edge case that could be addressed separately.

2023-08-16.14-18-57.mp4

Copy link
Contributor

@liamdebeasi liamdebeasi Aug 18, 2023

Choose a reason for hiding this comment

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

I think I had outdated code as I can no longer repro the issue I mentioned. However, another bug exists:

Switching to the month/year picker as the animation starts seems to break date selection altogether.

datetime-break.mov

Tested on Chrome 116. I added the following code to datetime/test/display/index.html to get the datetime to update async:

setTimeout(() => {
  datetime.value = '2023-10-23T16:30'
}, 2000);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm able to trigger that on main as well, using the next/prev month buttons. The animation goes through the same methods, so it's the same bug. Thoughts on addressing it separately?

2023-08-18.14-17-38.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that works for me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return;
}

/**
* If developers incorrectly use multiple="true"
* with non "date" datetimes, then just select
Expand Down
232 changes: 140 additions & 92 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 @@ -140,6 +136,17 @@ export class Datetime implements ComponentInterface {

@State() isTimePopoverOpen = false;

/**
* When defined, will force the datetime to render the month
* containing the specified date. Currently, this should only
* be used to enable immediately auto-scrolling to the new month,
* and should then be reset to undefined once the transition is
* finished and the forced month is now in view.
*
* Applies to grid-style datetimes only.
*/
@State() forceRenderDate?: DatetimeParts;

/**
* 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 @@ -221,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 @@ -302,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 @@ -356,54 +364,11 @@ export class Datetime implements ComponentInterface {
* Update the datetime value when the value changes
*/
@Watch('value')
protected valueChanged() {
const { value, minParts, maxParts, workingParts } = this;
protected async valueChanged() {
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);
}

this.emitStyle();
Expand Down Expand Up @@ -596,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 @@ -627,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 @@ -643,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 @@ -908,6 +864,20 @@ 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, assume we've
* scrolled to that and return it.
*
* If forceRenderDate is ever used in a context where the
* forced month is not immediately auto-scrolled to, this
* should be updated to also check whether `month` has the
* same month and year as the forced date.
*/
const { forceRenderDate } = this;
if (forceRenderDate !== undefined) {
return { month: forceRenderDate.month, year: forceRenderDate.year, day: forceRenderDate.day };
liamdebeasi marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* From here, we can determine if the start
* month or the end month was scrolled into view.
Expand Down Expand Up @@ -976,6 +946,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 @@ -1193,13 +1167,21 @@ export class Datetime implements ComponentInterface {
}

private processValue = (value?: string | string[] | null) => {
const hasValue = value !== null && value !== undefined;
const hasValue = value !== null && value !== undefined && (!Array.isArray(value) || value.length > 0);
Copy link
Contributor Author

@averyjohnston averyjohnston Jul 24, 2023

Choose a reason for hiding this comment

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

This change avoids a crash when manually setting a multiple selection datetime's value to [], by treating an empty array the same as setting the value to undefined (i.e. clearing the selected dates). This is done by the multiple test when the fixture's goto is called without passing in a default value.

I believe we weren't seeing the test crash before because processValue wasn't used when updating the value async, although I wouldn't be surprised if it was still possible to trigger this buggy behavior, just with different circumstances.

const valueToProcess = hasValue ? parseDate(value) : this.defaultParts;

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

this.warnIfIncorrectValueUsage();

/**
* Return early if the value wasn't parsed correctly, such as
* if an improperly formatted date string was provided.
*/
if (!valueToProcess) {
return;
}

/**
* Datetime should only warn of out of bounds values
* if set by the user. If the `value` is undefined,
Expand All @@ -1218,19 +1200,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 @@ -1258,6 +1232,67 @@ 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, and both are defined (otherwise there's nothing to animate to)
* 3. The calendar body is visible (prevents animation when in collapsed datetime-button, for example)
* 4. The month/year picker is not open (since you wouldn't see the animation anyway)
*/
const didChangeMonth =
(month !== undefined && month !== workingParts.month) || (year !== undefined && year !== workingParts.year);
const bodyIsVisible = el.classList.contains('datetime-ready');
const { isGridStyle, showMonthAndYear } = this;
if (isGridStyle && didChangeMonth && bodyIsVisible && !showMonthAndYear) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you set datetime to a time it still scrolls to another month and then abruptly resets to the selected month.

Example:

<ion-datetime value="2022-02-22T16:30:00"></ion-datetime>

<script>
setTimeout(() => {
  datetime.value = '16:30';
}, 2000);
</script>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed the wonky scrolling in fbf2f9c. The datetime now errors out if you do this, but it looks like this case (setting the value to just a time when the presentation is something other than just time) was never supported; I'm able to trigger the same error on main after messing with it a bit. (Screencast below.) I'm thinking it would be good to add a warning for this improper usage in a separate ticket. Thoughts?

2023-08-08.11-12-18.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.animateToDate(targetValue);
} else {
/**
* We only need to do this if we didn't just animate to a new month,
* since that calls prevMonth/nextMonth which calls setWorkingParts for us.
*/
this.setWorkingParts({
averyjohnston marked this conversation as resolved.
Show resolved Hide resolved
month,
day,
year,
hour,
minute,
ampm,
});
}
};

private animateToDate = async (targetValue: DatetimeParts) => {
const { workingParts } = this;

/**
* 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 = targetValue;

/**
* 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 = new Promise<void>((resolve) => {
this.resolveForceDateScrolling = resolve;
});

/**
* Animate smoothly to the forced month. This will also update
* workingParts and correct the surrounding months for us.
*/
const targetMonthIsBefore = isBefore(targetValue, workingParts);
targetMonthIsBefore ? this.prevMonth() : this.nextMonth();
await forceDateScrollingPromise;
this.resolveForceDateScrolling = undefined;
this.forceRenderDate = undefined;
};

componentWillLoad() {
Expand Down Expand Up @@ -1286,16 +1321,18 @@ export class Datetime implements ComponentInterface {
}
}

this.processMinParts();
this.processMaxParts();
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));

const todayParts = (this.todayParts = parseDate(getToday()));
const todayParts = (this.todayParts = parseDate(getToday())!);
this.defaultParts = getClosestValidDate(todayParts, monthValues, dayValues, yearValues, hourValues, minuteValues);

this.processMinParts();
this.processMaxParts();
Comment on lines +1333 to +1334
Copy link
Contributor Author

@averyjohnston averyjohnston Jul 24, 2023

Choose a reason for hiding this comment

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

I moved these further down to avoid an error where if a min or max is provided without a year, these functions would error out. This was because parseMinParts and parseMaxParts fall back to the current year (link), which is passed in via todayParts grabbed from the datetime. Previously, todayParts was being set after these functions were called, so it was still undefined.

I'm unsure why this wasn't causing problems before, but it was causing the minmax test to crash now.


this.processValue(this.value);

this.emitStyle();
Expand Down Expand Up @@ -2042,7 +2079,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 @@ -2151,7 +2188,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 @@ -2360,15 +2397,26 @@ export class Datetime implements ComponentInterface {
}

render() {
const { name, value, disabled, el, color, readonly, showMonthAndYear, preferWheel, presentation, size } = this;
const {
name,
value,
disabled,
el,
color,
readonly,
showMonthAndYear,
preferWheel,
presentation,
size,
isGridStyle,
} = this;
const mode = getIonMode(this);
const isMonthAndYearPresentation =
presentation === 'year' || presentation === 'month' || presentation === 'month-year';
const shouldShowMonthAndYear = showMonthAndYear || isMonthAndYearPresentation;
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 @@ -2387,7 +2435,7 @@ export class Datetime implements ComponentInterface {
[`datetime-presentation-${presentation}`]: true,
[`datetime-size-${size}`]: true,
[`datetime-prefer-wheel`]: hasWheelVariant,
[`datetime-grid`]: hasGrid,
[`datetime-grid`]: isGridStyle,
}),
}}
>
Expand Down
Loading