Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AnchoredOverlay and ActionList fixes for SelectPanel #1209

Merged
merged 5 commits into from
May 6, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/ActionList/Item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import styled from 'styled-components'
/**
* Contract for props passed to the `Item` component.
*/
export interface ItemProps extends React.ComponentPropsWithoutRef<'div'>, SxProp {
export interface ItemProps extends Omit<React.ComponentPropsWithoutRef<'div'>, 'id'>, SxProp {
/**
* Primary text which names an `Item`.
*/
Expand Down Expand Up @@ -69,6 +69,11 @@ export interface ItemProps extends React.ComponentPropsWithoutRef<'div'>, SxProp
* Callback that will trigger both on click selection and keyboard selection.
*/
onAction?: (item: ItemProps, event: React.MouseEvent<HTMLDivElement> | React.KeyboardEvent<HTMLDivElement>) => void

/**
* An id associated with this item. Should be unique between items
*/
id?: number | string
}

const getItemVariant = (variant = 'default', disabled?: boolean) => {
Expand Down Expand Up @@ -180,6 +185,7 @@ export function Item(itemProps: Partial<ItemProps> & {item?: ItemInput}): JSX.El
onKeyPress,
children,
onClick,
id,
...props
} = itemProps

Expand Down Expand Up @@ -215,6 +221,7 @@ export function Item(itemProps: Partial<ItemProps> & {item?: ItemInput}): JSX.El
variant={variant}
aria-selected={selected}
{...props}
data-id={id}
onKeyPress={keyPressHandler}
onClick={clickHandler}
>
Expand Down
15 changes: 5 additions & 10 deletions src/ActionList/List.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react'
import React, {useMemo} from 'react'
import type {AriaRole} from '../utils/types'
import {Group, GroupProps} from './Group'
import {Item, ItemProps} from './Item'
Expand Down Expand Up @@ -137,14 +137,8 @@ export function List(props: ListProps): JSX.Element {
*/
const renderItem = (itemProps: ItemInput, item: ItemInput) => {
const ItemComponent = ('renderItem' in itemProps && itemProps.renderItem) || props.renderItem || Item
return (
<ItemComponent
{...itemProps}
key={itemProps.key || uniqueId()}
sx={{...itemStyle, ...itemProps.sx}}
item={item}
/>
)
const key = itemProps.key ?? itemProps.id?.toString() ?? uniqueId()
return <ItemComponent {...itemProps} key={key} sx={{...itemStyle, ...itemProps.sx}} item={item} />
}

/**
Expand All @@ -153,10 +147,11 @@ export function List(props: ListProps): JSX.Element {
let groups: (GroupProps | (Partial<GroupProps> & {renderItem?: typeof Item; renderGroup?: typeof Group}))[] = []

// Collect rendered `Item`s into `Group`s, avoiding excess iteration over the lists of `items` and `groupMetadata`:
const singleGroupId = useMemo(uniqueId, [])
Copy link
Member

@smockle smockle May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, this is here (as opposed to preceding line 154) to ensure the same number of hooks is called on each render, i.e. to avoid conditionally calling useMemo as the number of Groups changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's exactly why. Hooks shouldn't be called inside loops or conditionals to ensure consistent execution order across renders.


if (!isGroupedListProps(props)) {
// When no `groupMetadata`s is provided, collect rendered `Item`s into a single anonymous `Group`.
groups = [{items: props.items?.map(item => renderItem(item, item)), groupId: uniqueId()}]
groups = [{items: props.items?.map(item => renderItem(item, item)), groupId: singleGroupId}]
} else {
// When `groupMetadata` is provided, collect rendered `Item`s into their associated `Group`s.

Expand Down
18 changes: 14 additions & 4 deletions src/AnchoredOverlay/AnchoredOverlay.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'
import Overlay from '../Overlay'
import Overlay, {OverlayProps} from '../Overlay'
import {useFocusTrap} from '../hooks/useFocusTrap'
import {useFocusZone} from '../hooks/useFocusZone'
import {useAnchoredPosition, useRenderForcingRef} from '../hooks'
Expand All @@ -9,7 +9,7 @@ function stopPropagation(event: React.UIEvent) {
event.stopPropagation()
}

export interface AnchoredOverlayProps {
export interface AnchoredOverlayProps extends Pick<OverlayProps, 'height' | 'width'> {
/**
* A custom function component used to render the anchor element.
* Will receive the selected text as `children` prop when an item is activated.
Expand All @@ -36,7 +36,15 @@ export interface AnchoredOverlayProps {
* An `AnchoredOverlay` provides an anchor that will open a floating overlay positioned relative to the anchor.
* The overlay can be opened and navigated using keyboard or mouse.
*/
export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({renderAnchor, children, open, onOpen, onClose}) => {
export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({
renderAnchor,
children,
open,
onOpen,
onClose,
height,
width
}) => {
const anchorRef = useRef<HTMLElement>(null)
const [overlayRef, updateOverlayRef] = useRenderForcingRef<HTMLDivElement>()
const [focusType, setFocusType] = useState<null | 'anchor' | 'list'>(open ? 'list' : null)
Expand Down Expand Up @@ -98,7 +106,7 @@ export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({renderAnchor, c
return position && {top: `${position.top}px`, left: `${position.left}px`}
}, [position])

useFocusZone({containerRef: overlayRef, disabled: !open || focusType !== 'list' || !position})
useFocusZone({containerRef: overlayRef, disabled: !open || !position})
useFocusTrap({containerRef: overlayRef, disabled: !open || focusType !== 'list' || !position})

return (
Expand All @@ -123,6 +131,8 @@ export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({renderAnchor, c
visibility={position ? 'visible' : 'hidden'}
onMouseDown={stopPropagation}
onClick={stopPropagation}
height={height}
width={width}
{...overlayPosition}
>
{children}
Expand Down