Skip to content

fix(datetime): allow calendar navigation in readonly mode; disallow keyboard navigation when disabled #28336

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
Merged
merged 22 commits into from
Oct 31, 2023
Merged

fix(datetime): allow calendar navigation in readonly mode; disallow keyboard navigation when disabled #28336

merged 22 commits into from
Oct 31, 2023

Conversation

mapsandapps
Copy link
Contributor

@mapsandapps mapsandapps commented Oct 11, 2023 •
edited
Loading

Issue number: #28121

What is the current behavior?

It is not possible to navigate between months when ion-datetime is in readonly mode. This means that if there are multiple dates selected, the user cannot browse to view them all.

Also, keyboard navigation is not prevented in readonly or disabled mode where it should be.

What is the new behavior?

When readonly:

  • Clicking the month-year button changes the month & year in readonly mode
  • Clicking the next & prev buttons changes the month in readonly mode
  • Left and right arrow keys change the month in readonly mode
  • Swiping/scrolling changes the month in readonly mode
  • The selected date does not change when doing any of the above
  • You cannot clear the value using keyboard navigation of the clear button in readonly mode

When disabled:

  • You cannot navigate months via keyboard navigation of the month-year button in disabled mode
  • You cannot navigate months using keyboard navigation of the previous & next buttons in disabled mode
  • You cannot navigate months via the left and right arrow keys in disabled mode
  • The selected date does not change when doing any of the above
  • You cannot clear the value using keyboard navigation of the clear button in disabled mode

Known bug:

  • It is still possible to navigate through dates in prefers-wheel when disabled. This bug existed prior to this PR. I created FW-5408 to track this.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added the package: core @ionic/core package label Oct 11, 2023
@mapsandapps mapsandapps marked this pull request as ready for review October 12, 2023 13:35
averyjohnston
averyjohnston suggested changes Oct 12, 2023
View reviewed changes
liamdebeasi
liamdebeasi suggested changes Oct 17, 2023
View reviewed changes
@mapsandapps mapsandapps changed the title fix(datetime): allow calendar navigation in readonly mode fix(datetime): allow calendar navigation in readonly mode; disallow keyboard navigation when disabled Oct 18, 2023
@mapsandapps mapsandapps marked this pull request as draft October 18, 2023 15:03
@mapsandapps mapsandapps marked this pull request as ready for review October 18, 2023 15:53
liamdebeasi
liamdebeasi suggested changes Oct 19, 2023
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.

We spoke offline about using the toBeDisabled assertion instead for many of these checks. This will let us consolidated tests and simplify the tests we end up keeping.

The implementation works well! I saw a few opportunities to clean up the code, but nothing major.

averyjohnston
averyjohnston suggested changes Oct 23, 2023
View reviewed changes
liamdebeasi
liamdebeasi approved these changes Oct 24, 2023
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'm fine with the changes as-is, but I would like to make sure one of the changes was intentional.

sean-perkins
sean-perkins suggested changes Oct 25, 2023
View reviewed changes
@brandyscarney brandyscarney removed their request for review October 25, 2023 21:27
sean-perkins
sean-perkins approved these changes Oct 27, 2023
View reviewed changes
averyjohnston
averyjohnston approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

Thanks for sorting through everything here!

@mapsandapps @averyjohnston
Update core/src/components/datetime/datetime.tsx
15ebec3 
Co-authored-by: Amanda Johnston <90629384+amandaejohnston@users.noreply.github.com>
@mapsandapps mapsandapps added this pull request to the merge queue Oct 31, 2023
Merged via the queue into main with commit f6a6877 Oct 31, 2023
@mapsandapps mapsandapps deleted the FW-5096 branch October 31, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@liamdebeasi liamdebeasi liamdebeasi approved these changes

@sean-perkins sean-perkins sean-perkins approved these changes

@averyjohnston averyjohnston averyjohnston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

package: core @ionic/core package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mapsandapps @liamdebeasi @sean-perkins @averyjohnston @Ionitron