feat: integrate column-option into column #28622

liamdebeasi · 2023-12-01T19:58:44Z

Issue number: Internal

What is the current behavior?

We'd like to introduce a new API to the picker column to allow developers to manipulate the picker options right in the DOM. Currently developers need to pass an object which is quite cumbersome as you need to re-pass that object anytime you want to change a single option.

What is the new behavior?

ion-picker-column now uses ion-picker-column-option for displaying options in a column
Scrolling the column and clicking an enabled option should update the value of the column/emit ionChange
Loading the picker with a valid value already set

There are a few things missing from this PR. I plan to address this separately and have disabled their tests for now:

Ensure column option is disabled if the parent column is disabled
Keyboard entry for numbers
Setting a value initially if nothing is set/the value is out of bounds -- There are several ways I can tackle this, but based on the feedback on this PR I may change my approach so figured it would be best to hold off for now.

Notes to reviewers:

There is likely duplicate/unneeded. I plan to focus on getting functionality implemented + tests passing before I go ripping other code out. I added TODO comments so I can keep track of this.
There should be no visual diffs as a result of this change
When updating the datetime tests I tried to rely on public APIs for our components as much as possible instead of diving into the Shadow DOM. My hope is that this reduces the need for test updates in the event we make more internal changes. Let me know if you have ideas on how I can improve the test changes in this PR!

Does this introduce a breaking change?

Yes
No

The shadow parts on the picker column options have been removed since they are in the light DOM now. The wheel-item and active parts remain when targeting the wheel inside of ion-datetime.

Other information

liamdebeasi · 2023-12-01T19:59:25Z

core/src/components/picker-column/picker-column.tsx

-          const dataIndex = activeElement.getAttribute('data-index');
-
-          /**
-           * If no value it is
-           * possible we hit one of the
-           * empty padding columns.
-           */
-          if (dataIndex === null) {
-            return;
-          }
-
-          const index = parseInt(dataIndex, 10);
-          const selectedItem = this.items[index];
-
-          if (selectedItem.value !== this.value) {
-            this.setValue(selectedItem.value);
-          }


Before, we were dealing with button elements so we had to encode the button index so we could then find the associated item value. We don't need to do that with the option component since we can just look at the value property directly.

liamdebeasi · 2023-12-01T20:00:06Z

core/src/components/picker-column/picker-column.tsx

@@ -325,8 +338,8 @@ export class PickerColumn implements ComponentInterface {
          }
        }

-        activeEl = activeElement;
-        this.setPickerItemActiveState(activeElement, true);
+        activeEl = newActiveElement;


The name change isn't needed to land this PR, but I couldn't immediately tell the different between activeEl and activeElement so I felt it best to rename.

Thank you! This makes it easier to comprehend when skimming the code.

liamdebeasi · 2023-12-01T20:02:20Z

core/src/components/picker-column/picker-column.tsx

+        /**
+         * This null check is important because activeEl
+         * can be undefined but newActiveElement can be
+         * null since we are using a querySelector.
+         * newActiveElement !== activeEl would return true
+         * below if newActiveElement was null but activeEl
+         * was undefined.
+         */
+        if (newActiveElement === null || newActiveElement.disabled) {


I considered making activeEl null instead of undefined, but I decided against it. Array.prototype.find returns undefined if an array option was not found, and querySelector returns null if the element was not found in the DOM. I decided it would be better to add this comment highlighting the importance of the newActiveElement === null check instead of having the other methods return a value they were not designed to return.

liamdebeasi · 2023-12-01T20:03:15Z

core/src/components/picker-column/picker-column.tsx

-    const findItem = items.find((item) => item.value === value && item.disabled !== true);
-    if (findItem) {
-      this.ionChange.emit(findItem);
-    }


Not needed anymore. We could have this filter through the existing ion-picker-column-option elements, but none of our other grouped components do this (accordion, radio group, etc)

liamdebeasi · 2023-12-01T20:18:21Z

core/src/components/picker-column-option/picker-column-option.tsx

+  componentDidLoad() {
+    const parentPickerColumn = this.el.closest('ion-picker-column');
+    if (parentPickerColumn !== null && this.value === parentPickerColumn.value) {
+      parentPickerColumn.scrollActiveItemIntoView();
+    }
+  }


This is new behavior. With the internal picker, the options were part of ion-picker-column-internal, so we knew exactly when the options were loaded. With the ion-picker-column-option approach, these options can be loaded at any time (such as in an *ngIf), so we need the options to tell the column when it is ready to be scrolled into view.

liamdebeasi · 2023-12-01T21:20:06Z

core/src/components/picker-column/test/disabled/picker-column.e2e.ts

- */
-configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
-  test.describe(title('picker-column: disabled rendering'), () => {
-    test('should not have visual regressions', async ({ page }) => {


This is checking that the column options are rendered correctly when disabled. I already did this in #28621, so this test is not needed.

liamdebeasi · 2023-12-01T21:20:22Z

core/src/components/picker-column/test/disabled/picker-column.e2e.ts

 /**
 * This behavior does not vary across modes/directions.
 */
 configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => {
  test.describe(title('picker-column: disabled items'), () => {
+    // TODO FW-5580 move this to a spec test in picker-column-option


This is much easier to test using a spec test now, so I'd like to update this in a future PR.

liamdebeasi · 2023-12-01T21:20:25Z

core/src/components/picker-column/test/disabled/picker-column.e2e.ts


      expect(await pickerItems.count()).toBe(3);
    });
+    // TODO FW-5580 move this to a spec test in picker-column-option


This is much easier to test using a spec test now, so I'd like to update this in a future PR.

liamdebeasi · 2023-12-01T21:20:52Z

core/src/components/picker-column/test/disabled/picker-column.e2e.ts

 configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => {
-  test.describe(title('picker-column: disabled column'), () => {
+  test.describe.skip(title('picker-column: disabled column'), () => {


This will be handled in a future PR

liamdebeasi · 2023-12-01T21:21:16Z

core/src/components/picker/test/keyboard-entry/picker.e2e.ts

 configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => {
-  test.describe(title('picker: keyboard entry'), () => {
+  test.describe.skip(title('picker: keyboard entry'), () => {


This will be handled in a future PR

liamdebeasi · 2023-12-01T21:41:01Z

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

@@ -53,31 +53,36 @@ <h2>Even items disabled</h2>
          <div class="grid-item">
            <h2>Column disabled</h2>
            <ion-picker>
-              <ion-picker-column id="column-disabled" value="11" disabled></ion-picker-column>


The value here was bound as a string, but the value on each option was bound as a number. This should have always been failing, but not sure why it wasn't in the past.

core/src/components/datetime/datetime.tsx

core/src/components/picker-column/picker-column.tsx

core/src/components/datetime/datetime.tsx

core/src/components/picker-column/picker-column.tsx

Co-authored-by: Sean Perkins <[email protected]>

core/src/components/picker-column-option/picker-column-option.tsx

thetaPC · 2023-12-06T18:14:55Z

core/src/components/picker-column/picker-column.tsx

@@ -325,8 +338,8 @@ export class PickerColumn implements ComponentInterface {
          }
        }

-        activeEl = activeElement;
-        this.setPickerItemActiveState(activeElement, true);
+        activeEl = newActiveElement;


Thank you! This makes it easier to comprehend when skimming the code.

core/src/components/picker/test/basic/picker.e2e.ts

core/src/components/picker/test/basic/index.html

Co-authored-by: Maria Hutt <[email protected]>

thetaPC

LGTM, smooth on click also works

liamdebeasi · 2023-12-06T21:30:35Z

Spoke with Sean offline and he's good with merging.

liamdebeasi and others added 7 commits December 1, 2023 12:44

feat(picker-column): add styles, disabled and active states

7b21bd4

chore(): add updated snapshots

9d0834b

chore: run build

1aeb194

refactor: add slot to integrate basic options

eace642

refactor: scrolling column sets value

b68c93d

refactor: clicking option sets value

2f3f9dc

chore: lint

75ee951

github-actions bot added the package: core @ionic/core package label Dec 1, 2023

liamdebeasi commented Dec 1, 2023

View reviewed changes

This comment was marked as spam.

Sign in to view

fix: column scrolls into view when option is ready

b1fc672

liamdebeasi commented Dec 1, 2023

View reviewed changes

liamdebeasi added 3 commits December 1, 2023 15:29

fix: do not emit ionChange if value did not change

2c773ed

build and lint

3470c64

add api

8d5a043

github-actions bot added the package: angular @ionic/angular package label Dec 1, 2023

liamdebeasi added 2 commits December 1, 2023 16:07

test: picker-column tests pass

34ec94f

test: picker tests pass

e933833

liamdebeasi commented Dec 1, 2023

View reviewed changes

liamdebeasi changed the title ~~Ld/5580 integration~~ feat: integrate column-option into column Dec 1, 2023

liamdebeasi added 2 commits December 1, 2023 16:35

add TODOs

fc36cc5

fix disabled test

58a89b3

liamdebeasi commented Dec 1, 2023

View reviewed changes

sean-perkins suggested changes Dec 5, 2023

View reviewed changes

liamdebeasi and others added 9 commits December 5, 2023 13:24

Update core/src/components/picker-column/picker-column.tsx

e8d5599

Co-authored-by: Sean Perkins <[email protected]>

use generic for query selector

d48c41d

remove explicit type since it can be inferred

e4797f7

remove explicit type case

a98e85d

remove typecast in favor of generic

33a21d3

remove unneeded typecast

0ef015b

remove explicit types

f9d01b9

active part no longer includes base part

cf3d028

lint

d36d645

liamdebeasi requested a review from sean-perkins December 5, 2023 18:40

Base automatically changed from ld/5580-base to FW-5580 December 5, 2023 19:40

liamdebeasi added 2 commits December 5, 2023 14:44

chore: sync

59bd18a

chore: resolve merge conflicts

fdfc5fc

liamdebeasi requested a review from a team as a code owner December 5, 2023 19:57

fix bad merge on api.txt

ca1b519

liamdebeasi mentioned this pull request Dec 6, 2023

fix(picker): keyboard entry works with options #28652

Merged

2 tasks

averyjohnston reviewed Dec 6, 2023

View reviewed changes

core/src/components/picker-column-option/picker-column-option.tsx Show resolved Hide resolved

liamdebeasi mentioned this pull request Dec 6, 2023

fix(picker): disabled column works with options #28653

Merged

2 tasks

thetaPC reviewed Dec 6, 2023

View reviewed changes

liamdebeasi and others added 2 commits December 6, 2023 14:55

Update core/src/components/picker-column-option/picker-column-option.tsx

0db0071

Co-authored-by: Maria Hutt <[email protected]>

fix: smooth scrolling when value changes

da2db5c

liamdebeasi requested review from thetaPC and averyjohnston December 6, 2023 20:04

build

4e59020

averyjohnston approved these changes Dec 6, 2023

View reviewed changes

thetaPC approved these changes Dec 6, 2023

View reviewed changes

liamdebeasi merged commit ebcc4be into FW-5580 Dec 6, 2023
44 checks passed

liamdebeasi deleted the ld/5580-integration branch December 6, 2023 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate column-option into column #28622

feat: integrate column-option into column #28622

liamdebeasi commented Dec 1, 2023 •

edited

Loading

liamdebeasi Dec 1, 2023

liamdebeasi Dec 1, 2023 •

edited

Loading

thetaPC Dec 6, 2023

liamdebeasi Dec 1, 2023

liamdebeasi Dec 1, 2023

This comment was marked as spam.

liamdebeasi Dec 1, 2023

liamdebeasi Dec 1, 2023

liamdebeasi Dec 1, 2023

liamdebeasi Dec 1, 2023

liamdebeasi Dec 1, 2023

liamdebeasi Dec 1, 2023

liamdebeasi Dec 1, 2023

thetaPC Dec 6, 2023

thetaPC left a comment

liamdebeasi commented Dec 6, 2023

feat: integrate column-option into column #28622

feat: integrate column-option into column #28622

Conversation

liamdebeasi commented Dec 1, 2023 • edited Loading

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Choose a reason for hiding this comment

liamdebeasi Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thetaPC left a comment

Choose a reason for hiding this comment

liamdebeasi commented Dec 6, 2023

liamdebeasi commented Dec 1, 2023 •

edited

Loading

liamdebeasi Dec 1, 2023 •

edited

Loading