fix(HoverDisclosure): Remove strategy prop, add placeholderWidth (#2510)

Author: Luke Bowerman

packages/components/src/List/ListItem.test.tsx

@@ -124,7 +124,7 @@ describe('ListItem', () => {
 
   test('renders icon', () => {
     renderWithTheme(<ListItem icon={<Science />}>Icon</ListItem>)
-    expect(screen.getByText('Icon')).toBeVisible()
+    expect(screen.getByText('Icon')).toBeInTheDocument()
   })
 
   test('renders artwork', () => {
@@ -266,9 +266,9 @@ describe('ListItem', () => {
       </ListItem>
     )
 
-    expect(screen.queryByText('Detail')).not.toBeVisible()
+    expect(screen.queryByText('Detail')).not.toBeInTheDocument()
     fireEvent.mouseEnter(screen.getByText('Label'), { bubbles: true })
-    expect(screen.queryByText('Detail')).toBeVisible()
+    expect(screen.queryByText('Detail')).toBeInTheDocument()
   })
 
   test('onKeyUp callback functions', () => {

packages/components/src/List/ListItem.tsx

@@ -233,9 +233,13 @@ const ListItemInternal = forwardRef(
       </Wrapper>
     )
 
-    const { accessory, content, hoverDisclosure, padding } = getDetailOptions(
-      detail
-    )
+    const {
+      accessory,
+      content,
+      hoverDisclosure,
+      padding,
+      width,
+    } = getDetailOptions(detail)
 
     const wrapperRef = useRef<HTMLLIElement | HTMLDivElement>(null)
     const actualRef = useForkedRef(wrapperRef, ref)
@@ -252,7 +256,7 @@ const ListItemInternal = forwardRef(
     })
 
     const renderedDetail = detail && (
-      <HoverDisclosure visible={!hoverDisclosure}>
+      <HoverDisclosure width={width} visible={!hoverDisclosure}>
         <ListItemDetail
           pl={padding ? 'xsmall' : '0'}
           pr={accessory && padding ? itemDimensions.px : '0'}

packages/components/src/List/types.ts

@@ -27,6 +27,7 @@
 import { FontSizes, LineHeights, SpacingSizes } from '@looker/design-tokens'
 import { ReactNode } from 'react'
 import { IconSize } from '../Icon'
+import { HoverDisclosureProps } from '../utils/HoverDisclosure'
 
 export type DensityRamp = -3 | -2 | -1 | 0 | 1
 
@@ -158,7 +159,7 @@ export type ListItemStatefulProps = {
   selected?: boolean
 }
 
-interface DetailOptions {
+interface DetailOptions extends Pick<HoverDisclosureProps, 'width'> {
   /**
    * If true, the detail will appear outside of the item's grey background on hover
    * In addition, if true, events originating from the detail will not bubble to the item's handlers

packages/components/src/List/utils/getDetailOptions.ts

@@ -28,17 +28,18 @@ import { Detail } from '../types'
 
 // Simplifies the type check when dealing with ListItem and related components
 export const getDetailOptions = (detail: Detail) => {
-  let accessory, padding, hoverDisclosure, content
+  let accessory, padding, hoverDisclosure, content, width
 
   if (typeof detail === 'object' && detail && 'options' in detail) {
     accessory = detail.options.accessory
     content = detail.content
     padding =
       detail.options.padding === undefined || detail.options.padding === true
     hoverDisclosure = detail.options.hoverDisclosure
+    width = detail.options.width
   } else {
     content = detail
   }
 
-  return { accessory, content, hoverDisclosure, padding }
+  return { accessory, content, hoverDisclosure, padding, width }
 }

packages/components/src/Tree/Tree.test.tsx

@@ -196,9 +196,9 @@ describe('Tree', () => {
       </Tree>
     )
 
-    expect(screen.queryByText('Tree Detail')).not.toBeVisible()
+    expect(screen.queryByText('Tree Detail')).not.toBeInTheDocument()
     fireEvent.mouseEnter(screen.getByText('Tree Label'), { bubbles: true })
-    expect(screen.queryByText('Tree Detail')).toBeVisible()
+    expect(screen.queryByText('Tree Detail')).toBeInTheDocument()
   })
 
   describe('color', () => {

packages/components/src/Tree/TreeItem.test.tsx

@@ -76,9 +76,9 @@ describe('TreeItem', () => {
       </TreeItem>
     )
 
-    expect(screen.queryByText('Detail')).not.toBeVisible()
+    expect(screen.queryByText('Detail')).not.toBeInTheDocument()
     fireEvent.mouseEnter(screen.getByText('Label'), { bubbles: true })
-    expect(screen.queryByText('Detail')).toBeVisible()
+    expect(screen.queryByText('Detail')).toBeInTheDocument()
   })
 
   test('Triggers onClickWhitespace when indent padding is clicked, itemRole = "none", and labelBackgroundOnly = true', () => {

packages/components/src/Tree/stories/FieldItem.tsx

@@ -105,6 +105,7 @@ export const FieldItem: FC<FieldItemProps> = ({
           accessory: true,
           hoverDisclosure: !isFieldMenuOpen,
           padding: false,
+          width: 48,
         },
       }}
       onKeyDown={(event) => {
@@ -130,7 +131,7 @@ export const FieldItem: FC<FieldItemProps> = ({
         >
           <Truncate>{children}</Truncate>
         </Flex>
-        <HoverDisclosure visible={isPivot} strategy="display">
+        <HoverDisclosure visible={isPivot}>
           <IconButton
             icon={<SubdirectoryArrowLeft />}
             onClick={() => setIsPivot(!isPivot)}
@@ -141,10 +142,7 @@ export const FieldItem: FC<FieldItemProps> = ({
             tooltipPlacement="top"
           />
         </HoverDisclosure>
-        <HoverDisclosure
-          visible={isFilter}
-          strategy={isPivot ? 'visibility' : 'display'}
-        >
+        <HoverDisclosure visible={isFilter} width={isPivot ? 24 : undefined}>
           <IconButton
             onClick={() => setIsFilter(!isFilter)}
             toggle={isFilter}

packages/components/src/utils/HoverDisclosure/HoverDisclosure.tsx

@@ -30,28 +30,24 @@ import { HoverDisclosureContext } from './HoverDisclosureContext'
 export interface HoverDisclosureProps {
   visible?: boolean
   /**
-   * Specify strategy to use for hiding content. Visibility is preferred
-   * as it is generally more accessible but `display` will _not_ reserve
-   * visible space when hidden and may be required when attempt to achieve
-   * certain layouts.
-   *
-   * @default 'visibility'
+   * In some circumstances it's required to reserve space for the hidden content
+   * before it is revealed. Specifying this will reserve a space of the specified
+   * width (in pixels)
    */
-  strategy?: 'visibility' | 'display'
+  width?: number
 }
 
 export const HoverDisclosure: FC<HoverDisclosureProps> = ({
   children,
-  strategy = 'visibility',
+  width,
   visible,
 }) => {
   const context = useContext(HoverDisclosureContext)
   const isVisible = visible || context.visible
 
-  const style: CSSProperties =
-    strategy === 'visibility'
-      ? { visibility: isVisible ? 'visible' : 'hidden' }
-      : { display: isVisible ? 'initial' : 'none' }
+  const style: CSSProperties = width
+    ? { flexBasis: width, flexShrink: 0, width }
+    : {}
 
-  return <div style={style}>{children}</div>
+  return <div style={style}>{isVisible ? children : null}</div>
 }