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.

Sign up for GitHub

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

Jump to bottom

fix(datetime): update active calendar display when value changes #24244

Merged
merged 6 commits into from
Nov 24, 2021
Merged

fix(datetime): update active calendar display when value changes #24244

merged 6 commits into from
Nov 24, 2021

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Nov 18, 2021 •
edited
Loading

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

When programmatically changing the value of ion-datetime, the visual display will not correctly reflect the updated value.

Issue Number: #24241

What is the new behavior?

Whenever the value is modified, the ion-datetime will correctly display the updated value.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added the package: core @ionic/core package label Nov 18, 2021
@sean-perkins sean-perkins force-pushed the FW-350 branch 2 times, most recently from 7d9cb56 to 232d60f Compare November 18, 2021 17:11
@sean-perkins sean-perkins added the v6 issues specific to Framework v6 label Nov 18, 2021
liamdebeasi
liamdebeasi reviewed Nov 18, 2021
View reviewed changes
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

I think this behavior should be discussed further. The concern I have with this change is if a user is interacting with the datetime on May 2020 and the value changes to June 2021, the entire view jumps to June 2021.

We had this behavior in the old Ionic 5 picker and some felt that it was a bad UX: #20016

Perhaps a middle ground could be to update the selected value without jumping the entire view to the new selected date? In this case, we could just update this.activeParts and leave this.workingParts alone. One thing I do notice is the ion-picker-column elements currently look at workingParts for the selected value. This might need to be changed to account for this.

@sean-perkins
Copy link
Contributor Author

Notes from aside conversation:

  1. We will update this PR (the ion-datepicker) to update the view for the selected value to reflect the new current date. This interaction will not scroll the view and the user will remain in place.
  2. In post 6.0, we will look into options that allow users to customize the scrolling behavior.

High level notes regarding different platforms/libraries around this topic:

  • iOS (native): will force scroll the user to the updated date when the value is changed
  • Android (flutter/dart/etc.): does not allow manipulation of the date picker reference, once it's opened all properties are readonly
  • Web (datetime-local) on device: selected date does not update in the calendar view when programmatically changed. User does not scroll to updated date.
  • Vuetify, Quasar, MUI: transitions to the current date, but if you are actively interacting with the time picker it will not. If you are interacting with the primary date picker, it will jump to the current date.

@sean-perkins
Copy link
Contributor Author

@liamdebeasi small tweak here after our conversation.

I found that when the presentation mode is time or date-time, it's justifiable to force the user's time picker to the current value. The calendar view shouldn't scroll based on previous conversation.

The time picker however, has no visual distinction between the current value and the active value being selected in the design spec. The path of least resistance is to update the time picker's representation immediately so that the user can see the current value and then modify from there.

Open to your thoughts/feedback 👍

liamdebeasi
liamdebeasi reviewed Nov 22, 2021
View reviewed changes
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Code looks good. Can we add an E2E test to verify this fix?

liamdebeasi
liamdebeasi suggested changes Nov 23, 2021
View reviewed changes
core/src/components/datetime/test/set-value/e2e.ts Show resolved Hide resolved
core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved
core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved
liamdebeasi
liamdebeasi approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Other than the variable name, this looks great. Nice job!

core/src/components/datetime/datetime.tsx Outdated Show resolved Hide resolved
@sean-perkins sean-perkins merged commit ec3bc52 into next Nov 24, 2021
@sean-perkins sean-perkins deleted the FW-350 branch November 24, 2021 16:23
This was referenced Jan 1, 2022
Closed
Closed
@averyjohnston averyjohnston mentioned this pull request Jul 18, 2023
Merged
2 tasks
averyjohnston added a commit that referenced this pull request Aug 23, 2023
@averyjohnston
fix(datetime): scroll to newly selected date when value changes (#27806)
32244fb 
Issue number: Resolves #26391

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

When updating the `value` programmatically on an `ion-datetime` after it
has already been created:
- With grid style: The selected date visually updates, but the calendar
does not scroll to the newly selected month.
- With wheel style: The selected date does not visually update, i.e. the
wheels do not move to show the newly selected date.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Grid style datetimes now scroll to the selected date using the same
animation as when clicking the next/prev month buttons.
- This animation mirrors the behavior in both MUI and native iOS. See
the [design
doc](https://github.com/ionic-team/ionic-framework-design-documents/blob/main/projects/ionic-framework/components/datetime/0003-datetime-async-value.md)
for more information and screen recordings.
- The animation will not occur if the month/year did not change, or when
the datetime is hidden.
- Wheel style datetimes now visually update to the selected date. No
animation occurs, also mirroring native.
- The `parseDate` util has also had its type signatures updated to
account for returning `undefined` when the date string is improperly
formatted. This was missed when the util was refactored to support
multiple date selection.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

- Docs PR: ionic-team/ionic-docs#3053
- While this can technically be considered a bug fix, we are merging it
into a feature branch for safety; it's a fairly significant change to
how datetime behaves, and may interfere with custom logic when updating
a datetime's value async.
- Jumping to the newly selected value is handled by replacing everything
[here](https://github.com/ionic-team/ionic-framework/pull/27806/files#diff-4a407530c60e3cf72bcc11acdd21c4803a94bf47ea81b99e757db1c93d2735b8L364-L407)
with `processValue()`. This covers both wheel and grid datetimes.
- `activePartsClone` as a whole was also removed. It was added in
#24244 to enable
changing `activeParts` without triggering a rerender (and thus jumping
to the new value) but since we now want to do that jump, the clone is no
longer needed.
- The animation code might be tricky to follow, so I recorded going
through it:
https://github.com/ionic-team/ionic-framework/assets/90629384/1afa5762-f493-441a-b662-f0429f2d86a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liamdebeasi liamdebeasi liamdebeasi approved these changes

Assignees
No one assigned
Labels
package: core @ionic/core package v6 issues specific to Framework v6
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sean-perkins @liamdebeasi