feat: Picker - Item description support

packages/code-studio/src/styleguide/Pickers.tsx

@@ -57,13 +57,23 @@ export function Pickers(): JSX.Element {
               <Text>Complex Ccc</Text>
             </Item>
           </Section>
-          <Section>
+          <Section key="Key B">
             <Item>Item Ddd</Item>
             <Item>Item Eee</Item>
             <Item textValue="Complex Fff">
               <PersonIcon />
               <Text>Complex Fff</Text>
             </Item>
+            <Item key="Ggg">
+              <PersonIcon />
+              <Text>Label</Text>
+              <Text slot="description">Description</Text>
+            </Item>
+            <Item key="Hhh">
+              <PersonIcon />
+              <Text>Label that causes overflow</Text>
+              <Text slot="description">Description that causes overflow</Text>
+            </Item>
           </Section>
         </Picker>
       </Flex>

packages/components/src/popper/Popper.scss

@@ -59,39 +59,60 @@ $animation-offset: 10px;
   transition: all $transition;
 }
 
-.popper-container[x-placement^='top'] > .popper-transition-enter,
-.popper-container[x-placement^='top'] > .popper-transition-exit {
+.popper-container[x-placement^='top']
+  > .spectrum-theme-provider
+  .popper-transition-enter,
+.popper-container[x-placement^='top']
+  > .spectrum-theme-provider
+  .popper-transition-exit {
   transform: scale($animation-scale) translate(0, $animation-offset);
 }
 
-.popper-container[x-placement^='right'] > .popper-transition-enter,
-.popper-container[x-placement^='right'] > .popper-transition-exit {
+.popper-container[x-placement^='right']
+  > .spectrum-theme-provider
+  .popper-transition-enter,
+.popper-container[x-placement^='right']
+  > .spectrum-theme-provider
+  .popper-transition-exit {
   transform: scale($animation-scale) translate(-$animation-offset, 0);
 }
 
-.popper-container[x-placement^='bottom'] > .popper-transition-enter,
-.popper-container[x-placement^='bottom'] > .popper-transition-exit {
+.popper-container[x-placement^='bottom']
+  > .spectrum-theme-provider
+  .popper-transition-enter,
+.popper-container[x-placement^='bottom']
+  > .spectrum-theme-provider
+  .popper-transition-exit {
   transform: scale($animation-scale) translate(0, -$animation-offset);
 }
 
-.popper-container[x-placement^='left'] > .popper-transition-enter,
-.popper-container[x-placement^='left'] > .popper-transition-exit {
+.popper-container[x-placement^='left']
+  > .spectrum-theme-provider
+  .popper-transition-enter,
+.popper-container[x-placement^='left']
+  > .spectrum-theme-provider
+  .popper-transition-exit {
   transform: scale($animation-scale) translate($animation-offset, 0);
 }
 
-.popper-container[x-placement^='top'] > .popper-transition-enter-active.popper,
+.popper-container[x-placement^='top']
+  > .spectrum-theme-provider
+  .popper-transition-enter-active.popper,
 .popper-container[x-placement^='right']
-  > .popper-transition-enter-active.popper,
+  > .spectrum-theme-provider
+  .popper-transition-enter-active.popper,
 .popper-container[x-placement^='bottom']
-  > .popper-transition-enter-active.popper,
+  > .spectrum-theme-provider
+  .popper-transition-enter-active.popper,
 .popper-container[x-placement^='left']
-  > .popper-transition-enter-active.popper {
+  > .spectrum-theme-provider
+  .popper-transition-enter-active.popper {
   opacity: 1;
   transform: none;
   transition: all $transition ease-out;
 }
 
-.popper-container[x-placement^='top'] > .popper {
+.popper-container[x-placement^='top'] > .spectrum-theme-provider .popper {
   margin-bottom: $arrow-width;
 
   .popper-arrow {
@@ -105,7 +126,7 @@ $animation-offset: 10px;
   }
 }
 
-.popper-container[x-placement^='bottom'] > .popper {
+.popper-container[x-placement^='bottom'] > .spectrum-theme-provider .popper {
   margin-top: $arrow-width;
 
   .popper-arrow {
@@ -119,7 +140,7 @@ $animation-offset: 10px;
   }
 }
 
-.popper-container[x-placement^='right'] > .popper {
+.popper-container[x-placement^='right'] > .spectrum-theme-provider .popper {
   margin-left: $arrow-width;
 
   .popper-arrow {
@@ -133,7 +154,7 @@ $animation-offset: 10px;
   }
 }
 
-.popper-container[x-placement^='left'] > .popper {
+.popper-container[x-placement^='left'] > .spectrum-theme-provider .popper {
   margin-right: $arrow-width;
 
   .popper-arrow {

packages/components/src/popper/Popper.tsx

@@ -23,6 +23,7 @@ import PopperJs, { PopperOptions, ReferenceObject } from 'popper.js';
 import PropTypes from 'prop-types';
 import ThemeExport from '../ThemeExport';
 import './Popper.scss';
+import { SpectrumThemeProvider } from '../theme/SpectrumThemeProvider';
 
 interface PopperProps {
   options: PopperOptions;
@@ -244,33 +245,35 @@ class Popper extends Component<PopperProps, PopperState> {
     const { show } = this.state;
 
     return (
-      <CSSTransition
-        in={show}
-        timeout={timeout}
-        classNames="popper-transition"
-        onEntered={this.handleEnter}
-        onExited={this.handleExit}
-      >
-        <div
-          onClick={e => {
-            // stop click events from escaping popper
-            e.stopPropagation();
-          }}
-          onKeyDown={e => {
-            if (e.key === 'Escape') this.hide();
-          }}
-          className={classNames('popper', { interactive }, className)}
-          onBlur={closeOnBlur ? this.handleBlur : undefined}
-          tabIndex={closeOnBlur ? -1 : undefined}
-          role="presentation"
+      <SpectrumThemeProvider isPortal>
+        <CSSTransition
+          in={show}
+          timeout={timeout}
+          classNames="popper-transition"
+          onEntered={this.handleEnter}
+          onExited={this.handleExit}
         >
-          <div className="popper-content">
-            {children}
-            {/* eslint-disable-next-line react/no-unknown-property */}
-            <div className="popper-arrow" x-arrow="" />
+          <div
+            onClick={e => {
+              // stop click events from escaping popper
+              e.stopPropagation();
+            }}
+            onKeyDown={e => {
+              if (e.key === 'Escape') this.hide();
+            }}
+            className={classNames('popper', { interactive }, className)}
+            onBlur={closeOnBlur ? this.handleBlur : undefined}
+            tabIndex={closeOnBlur ? -1 : undefined}
+            role="presentation"
+          >
+            <div className="popper-content">
+              {children}
+              {/* eslint-disable-next-line react/no-unknown-property */}
+              <div className="popper-arrow" x-arrow="" />
+            </div>
           </div>
-        </div>
-      </CSSTransition>
+        </CSSTransition>
+      </SpectrumThemeProvider>
     );
   }
 

packages/components/src/spectrum/picker/Picker.tsx

@@ -1,5 +1,6 @@
-import { Key, useCallback, useMemo } from 'react';
-import { Picker as SpectrumPicker } from '@adobe/react-spectrum';
+import { Key, ReactNode, useCallback, useMemo } from 'react';
+import { Flex, Picker as SpectrumPicker, Text } from '@adobe/react-spectrum';
+import { isElementOfType } from '@deephaven/react-hooks';
 import cl from 'classnames';
 import { Tooltip } from '../../popper';
 import {
@@ -50,6 +51,26 @@ export type PickerProps = {
   | 'defaultSelectedKey'
 >;
 
+/**
+ * Create tooltip content optionally wrapping with a Flex column for array
+ * content. This is needed for Items containing description `Text` elements.
+ */
+function createTooltipContent(content: ReactNode) {
+  if (typeof content === 'boolean') {
+    return String(content);
+  }
+
+  if (Array.isArray(content)) {
+    return (
+      <Flex direction="column" alignItems="start">
+        {content.filter(node => isElementOfType(node, Text))}
+      </Flex>
+    );
+  }
+
+  return content;
+}
+
 /**
  * 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,
@@ -84,11 +105,14 @@ export function Picker({
       // elements that back the Spectrum Picker. These are not visible in the UI,
       // but are used for accessibility purposes, so we set to an arbitrary
       // 'Empty' value so that they are not empty strings.
-      <Item key={key as Key} textValue={textValue === '' ? 'Empty' : textValue}>
+      <Item
+        key={key as Key}
+        textValue={textValue === '' || textValue == null ? 'Empty' : textValue}
+      >
         <PickerItemContent>{content}</PickerItemContent>
         {tooltipOptions == null || content === '' ? null : (
           <Tooltip options={tooltipOptions}>
-            {typeof content === 'boolean' ? String(content) : content}
+            {createTooltipContent(content)}
           </Tooltip>
         )}
       </Item>

packages/components/src/spectrum/picker/PickerItemContent.tsx

@@ -1,5 +1,7 @@
-import { isValidElement, ReactNode } from 'react';
+import { cloneElement, isValidElement, ReactNode } from 'react';
 import { Text } from '@adobe/react-spectrum';
+import cl from 'classnames';
+import { isElementOfType } from '@deephaven/react-hooks';
 import stylesCommon from '../../SpectrumComponent.module.scss';
 
 export interface PickerItemContentProps {
@@ -12,25 +14,57 @@ export interface PickerItemContentProps {
  */
 export function PickerItemContent({
   children: content,
-}: PickerItemContentProps): JSX.Element {
+}: PickerItemContentProps): JSX.Element | null {
   if (isValidElement(content)) {
     return content;
   }
 
+  /* eslint-disable no-param-reassign */
   if (content === '') {
     // Prevent the item height from collapsing when the content is empty
-    // eslint-disable-next-line no-param-reassign
-    content = <>&nbsp;</>;
-  }
-
-  if (typeof content === 'boolean') {
+    content = '\xa0'; // Non-breaking space
+  } else if (typeof content === 'boolean') {
     // Boolean values need to be stringified to render
-    // eslint-disable-next-line no-param-reassign
     content = String(content);
+  } else if (Array.isArray(content)) {
+    // For cases where there are multiple `Text` children, add a css class to
+    // handle overflow. The primary use case for multiple text nodes is when a
+    // description is provided for an item. e.g.
+    // <Item textValue="Some Text">
+    //   <SomeIcon />
+    //   <Text>Some Label</Text>
+    //   <Text slot="description">Some Description</Text>
+    // </Item>
+    content = content.map((item, i) =>
+      isElementOfType(item, Text)
+        ? cloneElement(item, {
+            ...item.props,
+            // `cloneElement` has the side effect of resetting React's internal
+            // `_store.validated` value to `false on the item. This causes it
+            // to be re-validated as a child in an array when is is rendered,
+            // even if the item was originally provided as an inline child.
+            // Since React expects array children to have explicit keys, this
+            // will show devtools warnings for items that wouldn't usually
+            // require explicit keys. Since we are only cloning `Text` nodes, it
+            // should be reasonable to fallback to a key matching the stringified
+            // content. The index suffix is an extra precation for when 2 <Text>
+            // nodes have the same value.
+            key: item.key ?? `${item.props.children}_${i}`,
+            UNSAFE_className: cl(
+              item.props.UNSAFE_className,
+              stylesCommon.spectrumEllipsis
+            ),
+          })
+        : item
+    );
   }
+  /* eslint-enable no-param-reassign */
 
-  return (
+  return typeof content === 'string' || typeof content === 'number' ? (
     <Text UNSAFE_className={stylesCommon.spectrumEllipsis}>{content}</Text>
+  ) : (
+    // eslint-disable-next-line react/jsx-no-useless-fragment
+    <>{content}</>
   );
 }
 

packages/components/src/spectrum/picker/PickerUtils.test.tsx

@@ -77,8 +77,6 @@ const expectedItems = {
     </Item>,
     {
       content: <span>No textValue</span>,
-      key: '',
-      textValue: '',
     },
   ],
   explicitKey: [
@@ -112,7 +110,6 @@ const expectedSections = {
   noTitle: [
     <Section>{expectedItems.singleStringChild[0]}</Section>,
     {
-      key: '',
       items: [expectedItems.singleStringChild[1]],
     },
   ],
@@ -226,7 +223,7 @@ describe('normalizeTooltipOptions', () => {
     [undefined, null],
     [null, null],
     [false, null],
-    [true, { placement: 'top-start' }],
+    [true, { placement: 'right' }],
     [{ placement: 'bottom-end' }, { placement: 'bottom-end' }],
   ] as const)('should return: %s', (options, expected) => {
     const actual = normalizeTooltipOptions(options);