From ac8b71e14753902de1620530e865a27ccf49c7ca Mon Sep 17 00:00:00 2001
From: Brian Ingles <brianingles@deephaven.io>
Date: Wed, 27 Mar 2024 10:27:48 -0500
Subject: [PATCH] Addressed review comments (#1858)

---
 packages/components/src/spectrum/picker/Picker.tsx | 14 +++++++-------
 .../src/spectrum/Picker/Picker.tsx                 | 14 ++++++++++++--
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/packages/components/src/spectrum/picker/Picker.tsx b/packages/components/src/spectrum/picker/Picker.tsx
index 7403636743..07dfb080dd 100644
--- a/packages/components/src/spectrum/picker/Picker.tsx
+++ b/packages/components/src/spectrum/picker/Picker.tsx
@@ -7,7 +7,11 @@ import {
   isElementOfType,
   usePopoverOnScrollRef,
 } from '@deephaven/react-hooks';
-import { PICKER_ITEM_HEIGHT, PICKER_TOP_OFFSET } from '@deephaven/utils';
+import {
+  EMPTY_FUNCTION,
+  PICKER_ITEM_HEIGHT,
+  PICKER_TOP_OFFSET,
+} from '@deephaven/utils';
 import cl from 'classnames';
 import { Tooltip } from '../../popper';
 import {
@@ -45,7 +49,7 @@ export type PickerProps = {
    */
   onChange?: (key: PickerItemKey) => void;
 
-  /** Handler that is called when the popover is scrolled. */
+  /** Handler that is called when the picker is scrolled. */
   onScroll?: (event: Event) => void;
 
   /**
@@ -87,10 +91,6 @@ function createTooltipContent(content: ReactNode) {
   return content;
 }
 
-function noOp(): void {
-  // No-op
-}
-
 /**
  * Picker component for selecting items from a list of items. Items can be
  * provided via the `items` prop or as children. Each item can be a string,
@@ -106,7 +106,7 @@ export function Picker({
   getInitialScrollPosition,
   onChange,
   onOpenChange,
-  onScroll = noOp,
+  onScroll = EMPTY_FUNCTION,
   onSelectionChange,
   // eslint-disable-next-line camelcase
   UNSAFE_className,
diff --git a/packages/jsapi-components/src/spectrum/Picker/Picker.tsx b/packages/jsapi-components/src/spectrum/Picker/Picker.tsx
index ece2507f87..028cad09c2 100644
--- a/packages/jsapi-components/src/spectrum/Picker/Picker.tsx
+++ b/packages/jsapi-components/src/spectrum/Picker/Picker.tsx
@@ -19,6 +19,8 @@ export interface PickerProps extends Omit<PickerPropsBase, 'children'> {
   keyColumn?: string;
   /* The column of values to display as primary text. Defaults to the `keyColumn` value. */
   labelColumn?: string;
+
+  // TODO #1890 : descriptionColumn, iconColumn
 }
 
 export function Picker({
@@ -77,11 +79,19 @@ export function Picker({
     // Set viewport to include the selected item so that its data will load and
     // the real `key` will be available to show the selection in the UI.
     function setViewportFromSelectedKey() {
+      let isCanceled = false;
+
       getItemIndexByValue().then(index => {
-        if (index != null) {
-          setViewport(index);
+        if (index == null || isCanceled) {
+          return;
         }
+
+        setViewport(index);
       });
+
+      return () => {
+        isCanceled = true;
+      };
     },
     [getItemIndexByValue, setViewport]
   );