fix(datetime): allow disabling datetime with prefer-wheel by mapsandapps · Pull Request #28511 · ionic-team/ionic-framework

mapsandapps · 2023-11-10T16:06:20Z

Issue number: Internal

What is the current behavior?

It is possible to navigate the columns of a disabled Datetime with prefer-wheel via the keyboard.

disabled-bug.mov

What is the new behavior?

Picker Column Internal has a disabled state that disables the full column
When a Datetime is disabled with prefer-wheel, the columns in the Datetime will be disabled
It is no longer possible to navigate the wheels in a disabled Datetime via the keyboard

Comparison of native & Ionic components:

Does this introduce a breaking change?

Yes
No

sean-perkins · 2023-11-13T18:12:45Z

+      });
+      await page.waitForChanges();
+
+      await expect(disabledColumn).toHaveScreenshot(screenshot('picker-internal-disabled-column'));


We should dig into this test more. Playwright is already reporting it as flaky.

It's still flaking. I can't reproduce locally 😕

Can we try loading the test with the value already set? The problem might be that Playwright isn't waiting for the scroll to finish before taking a screenshot.

Done in 4661667 and I also made a change in 7a3771a that hopefully will help.

I meant that we should try loading the picker with value="11" already set. So you should be able to do value="11" here:

ionic-framework/core/src/components/picker-column-internal/test/disabled/index.html

Line 56 in 4661667

<ion-picker-column-internal id="column-disabled" value="12" disabled></ion-picker-column-internal>

and then remove this line:

ionic-framework/core/src/components/picker-column-internal/test/disabled/index.html

Line 81 in 4661667

fullDisabledPicker.value = 11;

For 7a3771a this causes a regression where the selected item is no longer highlighted when the picker column is disabled. You can see this at src/components/picker-column-internal/test/disabled on MD mode. The "11" option in "Column disabled" is no longer highlighted in blue.

According to your comment in https://github.com/ionic-team/ionic-framework/pull/28511/files#diff-5e7b473017ccb3a8be0fa9c7c9d8e1fc393b64ddfe719d5f3274e212ba09b3fdR416 this item should be highlighted blue.

I don't understand how setting the value directly changes anything for the scrolling if the items don't get set until the script? It can't scroll until the items are there any more than it can scroll if the value isn't there, right? 🤔

My comment about the active item was referring to the fact that the item should be scrolled to. But from looking at our other disabled md components, it does look like it should be blue as well. I'll revert that change, but I don't know what's causing it to be blue sometimes and gray others in md mode on Safari.

Updated. Also, I added a screenshot for the disabled calendar wheel to capture that intended behavior.

I don't understand how setting the value directly changes anything for the scrolling if the items don't get set until the script? It can't scroll until the items are there any more than it can scroll if the value isn't there, right?

The issue that I raised was the value was being set after the component loaded: https://github.com/ionic-team/ionic-framework/pull/28511/files/b7e42befeabbbb96044bbaa0aeb6ba6668baa547#diff-875e8dba2863da31c37c64b445f9c01418f2ee542625acf814f7a8cab7464b19R174

Your old test loaded the picker with value="12". The picker spends time scrolling to value 12. The test then set the value to 11, so the picker then needed to spend additional time scrolling to value 11. This additional value update can take a few hundred ms especially on CI.

By setting up the test with the correct configuration on load we a) reduce the run time of the test and b) reduce the likelihood of flaky tests.

sean-perkins

Behavior looks great locally 👍 I had one suggestion for organizing the styles, but everything else looks good to me. Great work!

liamdebeasi

The behavior looks good now, though there are some places where the code can be cleaned up.

liamdebeasi · 2023-11-21T14:37:38Z

        class={createColorClasses(color, {
          [mode]: true,
          ['picker-column-active']: isActive,
+          ['picker-column-disabled']: pickerDisabled,


This class doesn't seem needed anymore. The only place it's used is in the CSS, and that could be swapped out for the [disabled] attribute.

Sure. I went ahead and removed the .picker-item-disabled class while I was in there, by the same reasoning, even though it predates this PR.

~~I was referring to picker-column-disabled not picker-item-disabled. The picker-column-disabled class was added in this PR.~~

edit: see below

Re-read this and looks like I misunderstood your original comment. All of that sounds good!

liamdebeasi

I made one change in fab89a7

This test is not needed because we already have the following tests:

A test to verify that datetime sets the disabled prop on columns correctly: https://github.com/ionic-team/ionic-framework/pull/28511/files#diff-68455eb0492521a5359f114d63eedc2c4d3731e47f7fa6f349ba54456bcc18a2
A test to verify that a column renders correctly when disabled is set: https://github.com/ionic-team/ionic-framework/pull/28511/files#diff-875e8dba2863da31c37c64b445f9c01418f2ee542625acf814f7a8cab7464b19

Datetime does not apply any disabled-specific visual styles for the wheel picker. It does apply pointer-events: none, but the test I removed was not checking that.

The rest of this is good to go. Great work!

fix(datetime): allow disabling datetime with prefer-wheel

ed7f7a8

github-actions Bot added the package: core @ionic/core package label Nov 10, 2023

chore(): add updated snapshots

b7e42be

mapsandapps marked this pull request as ready for review November 13, 2023 14:17

mapsandapps requested review from averyrousseau, liamdebeasi and sean-perkins as code owners November 13, 2023 14:17

sean-perkins suggested changes Nov 13, 2023

View reviewed changes

mapsandapps added 5 commits November 13, 2023 17:59

Merge branch 'main' into FW-5408

254294e

Convert test

be8f483

Improve test (still failing)

41b6369

Fix test

031e9ab

Merge branch 'main' into FW-5408

0bab06e

sean-perkins approved these changes Nov 15, 2023

View reviewed changes

Comment thread core/src/components/picker-column-internal/picker-column-internal.scss Outdated

mapsandapps added 2 commits November 15, 2023 14:25

Rearrange styles

aff808a

Merge branch 'main' into FW-5408

eb3dd1d

liamdebeasi reviewed Nov 16, 2023

View reviewed changes

Comment thread core/src/components/picker-column-internal/picker-column-internal.scss Outdated

Comment thread core/src/components/picker-column-internal/picker-column-internal.tsx

mapsandapps added 5 commits November 16, 2023 17:59

Merge branch 'main' into FW-5408

c5c6405

Refactor & address PR feedback

b2b4116

Fix refactoring typo

549a11f

Make IntersectionObserver mocking more consistent

9759820

Fix styles

8477187

mapsandapps requested a review from liamdebeasi November 17, 2023 21:41

liamdebeasi reviewed Nov 20, 2023

View reviewed changes

Comment thread core/src/components/picker-column-internal/picker-column-internal.tsx Outdated

mapsandapps added 2 commits November 20, 2023 14:02

Simplify selector

d8d04be

Address PR feedback

450448c

liamdebeasi suggested changes Nov 21, 2023

View reviewed changes

mapsandapps added 3 commits November 21, 2023 11:02

Address PR feedback

048bb82

Address PR feedback

b15fbfa

Try to fix flaky test

7a3771a

Try to fix test by setting value initially

4661667

mapsandapps requested a review from liamdebeasi November 21, 2023 23:21

mapsandapps and others added 5 commits November 21, 2023 18:31

Revert 7a3771a

d49a495

Set value on element

845ecb3

Add snapshot test for disabled datetime with wheel

c86735f

chore(): add updated snapshots

ffec723

test: remove redundant test

fab89a7

liamdebeasi approved these changes Nov 22, 2023

View reviewed changes

liamdebeasi enabled auto-merge November 22, 2023 16:05

liamdebeasi added this pull request to the merge queue Nov 22, 2023

Merged via the queue into main with commit 01130e1 Nov 22, 2023

liamdebeasi deleted the FW-5408 branch November 22, 2023 16:18

Uh oh!

Conversation

mapsandapps commented Nov 10, 2023

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sean-perkins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liamdebeasi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamdebeasi Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamdebeasi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

liamdebeasi left a comment •

edited

Loading

liamdebeasi Nov 21, 2023 •

edited

Loading