Skip to content

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

Merged
mapsandapps merged 22 commits into
mainfrom
FW-5096
Oct 31, 2023
Merged

fix(datetime): allow calendar navigation in readonly mode; disallow keyboard navigation when disabled#28336
mapsandapps merged 22 commits into
mainfrom
FW-5096

Conversation

@mapsandapps

@mapsandapps mapsandapps commented Oct 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
averyrousseau
averyrousseau suggested changes Oct 12, 2023
View reviewed changes
Comment thread core/src/components/datetime/datetime.tsx
liamdebeasi
liamdebeasi suggested changes Oct 17, 2023
View reviewed changes
Comment thread core/src/components/datetime/datetime.tsx
Comment thread core/src/components/datetime/datetime.tsx
@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

@liamdebeasi liamdebeasi left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread core/src/components/datetime/datetime.tsx Outdated
Comment thread core/src/components/datetime/datetime.tsx Outdated
Comment thread core/src/components/datetime/datetime.scss Outdated
Comment thread core/src/components/datetime/test/disabled/datetime.e2e.ts Outdated
Comment thread core/src/components/datetime/test/disabled/datetime.e2e.ts Outdated
Comment thread core/src/components/datetime/test/disabled/datetime.e2e.ts Outdated
Comment thread core/src/components/datetime/test/disabled/datetime.e2e.ts Outdated
Comment thread core/src/components/datetime/test/disabled/datetime.e2e.ts Outdated
Comment thread core/src/components/datetime/test/disabled/datetime.e2e.ts Outdated
averyrousseau
averyrousseau suggested changes Oct 23, 2023
View reviewed changes
Comment thread core/src/components/datetime/datetime.tsx
Comment thread core/src/components/datetime/test/disabled/datetime.e2e.ts Outdated
Comment thread core/src/components/datetime/test/readonly/datetime.e2e.ts Outdated
liamdebeasi
liamdebeasi approved these changes Oct 24, 2023
View reviewed changes

@liamdebeasi liamdebeasi left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread core/src/components/datetime/datetime.scss
sean-perkins
sean-perkins suggested changes Oct 25, 2023
View reviewed changes
Comment thread core/src/components/datetime/datetime.tsx Outdated
Comment thread core/src/components/datetime/datetime.tsx Outdated
Comment thread core/src/components/datetime/datetime.tsx Outdated
@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
Comment thread core/src/components/datetime/datetime.tsx Outdated
averyrousseau
averyrousseau approved these changes Oct 30, 2023
View reviewed changes

@averyrousseau averyrousseau left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread core/src/components/datetime/datetime.tsx Outdated
@mapsandapps @averyrousseau
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
@averyrousseau averyrousseau averyrousseau 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 @averyrousseau @Ionitron