From 7c7fb2b1a3bf35b123716b2f975231ceb01dcc07 Mon Sep 17 00:00:00 2001
From: Maria Hutt <thetaPC@users.noreply.github.com>
Date: Thu, 11 May 2023 08:23:45 -0700
Subject: [PATCH] fix(picker-column): dynamically change options (#27359)

Issue number: resolves #21763

---------

<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

<!-- 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. -->

Picker options do not render correctly when dynamically changed. It
throws an error when the original options' length is no longer the same
as the updated options' length. This is due to `refresh()` being called
with `this.optsEl` having a stale list of children. The list doesn't get
updated until the upcoming render.


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

The column will call `refresh()` when it detects the columns has
changed. The call needs to be done after `this.optsEl` gets updated with
the correct number of children.

`componentShouldUpdate()` will check if options has changed ->
re-renders with the updated options -> `componentDidUpdate()` calls
`refresh()` based on `componentShouldUpdate()`

The standalone test page has been updated to include a way to test this
fix.


## 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. -->

It might be beneficial to review the [component
lifecycle](https://stenciljs.com/docs/component-lifecycle).

Co-authored-by: liamdebeasi <liamdebeasi@users.noreply.github.com>

---------

Co-authored-by: ionitron <hi@ionicframework.com>
---
 .../picker-column/picker-column.tsx           | 50 +++++++++++++++----
 .../test/picker-column-dynamic.spec.tsx       | 32 ++++++++++++
 2 files changed, 72 insertions(+), 10 deletions(-)
 create mode 100644 core/src/components/picker-column/test/picker-column-dynamic.spec.tsx

diff --git a/core/src/components/picker-column/picker-column.tsx b/core/src/components/picker-column/picker-column.tsx
index 4eaf391afa..d62e7cc8af 100644
--- a/core/src/components/picker-column/picker-column.tsx
+++ b/core/src/components/picker-column/picker-column.tsx
@@ -33,6 +33,14 @@ export class PickerColumnCmp implements ComponentInterface {
   private rafId?: ReturnType<typeof requestAnimationFrame>;
   private tmrId?: ReturnType<typeof setTimeout>;
   private noAnimate = true;
+  // `colDidChange` is a flag that gets set when the column is changed
+  // dynamically. When this flag is set, the column will refresh
+  // after the component re-renders to incorporate the new column data.
+  // This is necessary because `this.refresh` queries for the option elements,
+  // so it needs to wait for the latest elements to be available in the DOM.
+  // Ex: column is created with 3 options. User updates the column data
+  // to have 5 options. The column will still think it only has 3 options.
+  private colDidChange = false;
 
   @Element() el!: HTMLElement;
 
@@ -46,7 +54,7 @@ export class PickerColumnCmp implements ComponentInterface {
   @Prop() col!: PickerColumn;
   @Watch('col')
   protected colChanged() {
-    this.refresh();
+    this.colDidChange = true;
   }
 
   async connectedCallback() {
@@ -74,21 +82,32 @@ export class PickerColumnCmp implements ComponentInterface {
       onEnd: (ev) => this.onEnd(ev),
     });
     this.gesture.enable();
+    // Options have not been initialized yet
+    // Animation must be disabled through the `noAnimate` flag
+    // Otherwise, the options will render
+    // at the top of the column and transition down
     this.tmrId = setTimeout(() => {
       this.noAnimate = false;
+      // After initialization, `refresh()` will be called
+      // At this point, animation will be enabled. The options will
+      // animate as they are being selected.
       this.refresh(true);
     }, 250);
   }
 
   componentDidLoad() {
-    const colEl = this.optsEl;
-    if (colEl) {
-      // DOM READ
-      // We perfom a DOM read over a rendered item, this needs to happen after the first render
-      this.optHeight = colEl.firstElementChild ? colEl.firstElementChild.clientHeight : 0;
-    }
+    this.onDomChange();
+  }
 
-    this.refresh();
+  componentDidUpdate() {
+    // Options may have changed since last update.
+    if (this.colDidChange) {
+      // Animation must be disabled through the `onDomChange` parameter.
+      // Otherwise, the recently added options will render
+      // at the top of the column and transition down
+      this.onDomChange(true, false);
+      this.colDidChange = false;
+    }
   }
 
   disconnectedCallback() {
@@ -331,7 +350,7 @@ export class PickerColumnCmp implements ComponentInterface {
     }
   }
 
-  private refresh(forceRefresh?: boolean) {
+  private refresh(forceRefresh?: boolean, animated?: boolean) {
     let min = this.col.options.length - 1;
     let max = 0;
     const options = this.col.options;
@@ -356,11 +375,22 @@ export class PickerColumnCmp implements ComponentInterface {
     const selectedIndex = clamp(min, this.col.selectedIndex ?? 0, max);
     if (this.col.prevSelected !== selectedIndex || forceRefresh) {
       const y = selectedIndex * this.optHeight * -1;
+      const duration = animated ? TRANSITION_DURATION : 0;
       this.velocity = 0;
-      this.update(y, TRANSITION_DURATION, true);
+      this.update(y, duration, true);
     }
   }
 
+  private onDomChange(forceRefresh?: boolean, animated?: boolean) {
+    const colEl = this.optsEl;
+    if (colEl) {
+      // DOM READ
+      // We perfom a DOM read over a rendered item, this needs to happen after the first render or after the the column has changed
+      this.optHeight = colEl.firstElementChild ? colEl.firstElementChild.clientHeight : 0;
+    }
+    this.refresh(forceRefresh, animated);
+  }
+
   render() {
     const col = this.col;
     const mode = getIonMode(this);
diff --git a/core/src/components/picker-column/test/picker-column-dynamic.spec.tsx b/core/src/components/picker-column/test/picker-column-dynamic.spec.tsx
new file mode 100644
index 0000000000..f0f5fbe757
--- /dev/null
+++ b/core/src/components/picker-column/test/picker-column-dynamic.spec.tsx
@@ -0,0 +1,32 @@
+import { h } from '@stencil/core';
+import { newSpecPage } from '@stencil/core/testing';
+
+import { PickerColumnCmp } from '../picker-column';
+
+describe('picker-column: dynamic options', () => {
+  /**
+   * Issue: https://github.com/ionic-team/ionic-framework/issues/21763
+   */
+  it('should add an option', async () => {
+    const defaultOptions = [
+      { text: 'Dog', value: 'dog' },
+      { text: 'Cat', value: 'cat' },
+    ];
+
+    const page = await newSpecPage({
+      components: [PickerColumnCmp],
+      template: () => <ion-picker-column col={{ options: defaultOptions }}></ion-picker-column>,
+    });
+
+    const pickerCol = page.body.querySelector('ion-picker-column');
+
+    pickerCol.col = {
+      options: [...defaultOptions, { text: 'Carrot', value: 'carrot' }],
+    };
+
+    await page.waitForChanges();
+
+    const pickerOpt = pickerCol.querySelector('.picker-opt:nth(2)');
+    expect(pickerOpt.getAttribute('style')).toContain('transform');
+  });
+});