diff --git a/.changeset/serious-adults-protect.md b/.changeset/serious-adults-protect.md new file mode 100644 index 00000000000..7719127277f --- /dev/null +++ b/.changeset/serious-adults-protect.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +TreeView: Allow expanded state to be controlled diff --git a/.eslintrc.json b/.eslintrc.json index 1bc5cd1700e..3b81f7bd1cc 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -45,6 +45,7 @@ }, // rules which apply to JS, TS, etc. "rules": { + "no-shadow": 0, "react/prop-types": 0, "react/display-name": 0, "react-hooks/exhaustive-deps": "error", @@ -69,7 +70,6 @@ "rules": { "eslint-comments/no-use": 0, "import/no-namespace": 0, - "no-shadow": 0, "no-unused-vars": [ "error", { @@ -94,6 +94,14 @@ { "argsIgnorePattern": "^_" } + ], + "@typscript-eslint/no-shadow": 0, + "@typescript-eslint/no-empty-function": 0, + "@typescript-eslint/ban-ts-comment": [ + "error", + { + "ts-ignore": "allow-with-description" + } ] } }, diff --git a/docs/content/TreeView.mdx b/docs/content/TreeView.mdx index 2612c20203d..d39e08369d6 100644 --- a/docs/content/TreeView.mdx +++ b/docs/content/TreeView.mdx @@ -69,6 +69,35 @@ description: A hierarchical list of items where nested items can be expanded and ``` +### With controlled expanded state + +```javascript live noinline drafts +function ControlledTreeView() { + const [expanded, setExpanded] = React.useState(false) + + return ( + + + + + ) +} + +render() +``` + ## Props ### TreeView @@ -82,18 +111,31 @@ description: A hierarchical list of items where nested items can be expanded and - + + + - - {/* */} @@ -113,11 +155,31 @@ description: A hierarchical list of items where nested items can be expanded and } /> + + + + - {/* */} diff --git a/src/PageLayout/PageLayout.tsx b/src/PageLayout/PageLayout.tsx index 7f0ca096576..56c1e1fa867 100644 --- a/src/PageLayout/PageLayout.tsx +++ b/src/PageLayout/PageLayout.tsx @@ -77,7 +77,6 @@ const Root: React.FC> = ({ ( console.log(`${isExpanded ? 'Expanded' : 'Collapsed'} "public" folder `)} + onExpandedChange={isExpanded => console.log(`${isExpanded ? 'Expanded' : 'Collapsed'} "public" folder `)} > public @@ -67,7 +70,7 @@ export const FileTreeWithoutDirectoryLinks: Story = () => { console.log(`${isExpanded ? 'Expanded' : 'Collapsed'} "public" folder `)} + onExpandedChange={isExpanded => console.log(`${isExpanded ? 'Expanded' : 'Collapsed'} "public" folder `)} > public @@ -82,4 +85,215 @@ export const FileTreeWithoutDirectoryLinks: Story = () => { ) } +type TreeItem = { + data: { + name: string + expanded: boolean + } + children: TreeItem[] +} + +function expandAll(tree: TreeItem[]): TreeItem[] { + return tree.map(item => ({ + data: { + ...item.data, + expanded: true + }, + children: expandAll(item.children) + })) +} + +function collapseAll(tree: TreeItem[]): TreeItem[] { + return tree.map(item => ({ + data: { + ...item.data, + expanded: false + }, + children: collapseAll(item.children) + })) +} + +function setExpanded(tree: TreeItem[], path: string[], expanded: boolean): TreeItem[] { + return tree.map(item => { + if (item.data.name === path[0] && path.length === 1) { + return { + ...item, + data: { + ...item.data, + expanded + } + } + } else if (item.data.name === path[0]) { + return { + ...item, + children: setExpanded(item.children, path.slice(1), expanded) + } + } else { + return item + } + }) +} + +const CurrentPathContext = React.createContext<{ + currentPath: string[] + setCurrentPath: React.Dispatch> +}>({ + currentPath: [], + setCurrentPath: () => {} +}) + +export const Controlled: Story = () => { + const [currentPath, setCurrentPath] = React.useState(['src', 'Avatar.tsx']) + const [tree, setTree] = React.useState([ + { + data: { + name: 'src', + expanded: false + }, + children: [ + { + data: { + name: 'Avatar.tsx', + expanded: false + }, + children: [] + }, + { + data: { + name: 'Button', + expanded: false + }, + children: [ + { + data: { + name: 'Button.tsx', + expanded: false + }, + children: [] + }, + { + data: { + name: 'Button.test.tsx', + expanded: false + }, + children: [] + } + ] + } + ] + }, + { + data: { + name: 'public', + expanded: false + }, + children: [ + { + data: { + name: 'index.html', + expanded: false + }, + children: [] + }, + { + data: { + name: 'favicon.ico', + expanded: false + }, + children: [] + } + ] + }, + { + data: { + name: 'package.json', + expanded: false + }, + children: [] + } + ]) + + return ( + + + + + + Jump to + + + + setCurrentPath(['src'])}>src + setCurrentPath(['src', 'Avatar.tsx'])}>src/Avatar.tsx + setCurrentPath(['src', 'Button'])}>src/Button + setCurrentPath(['src', 'Button', 'Button.tsx'])}> + src/Button/Button.tsx + + setCurrentPath(['src', 'Button', 'Button.test.tsx'])}> + src/Button/Button.test.tsx + + setCurrentPath(['public'])}>public + setCurrentPath(['public', 'index.html'])}> + public/index.html + + setCurrentPath(['public', 'favicon.ico'])}> + public/favicon.ico + + setCurrentPath(['package.json'])}>package.json + + + + + + + ) +} + +function TreeItem({ + item, + path, + onExpandedChange +}: { + item: TreeItem + path: string[] + onExpandedChange: (path: string[], expanded: boolean) => void +}) { + const {currentPath, setCurrentPath} = React.useContext(CurrentPathContext) + return ( + onExpandedChange(path, expanded)} + onSelect={() => setCurrentPath(path)} + > + {item.data.name} + {item.children.length > 0 ? ( + + {item.children.map(child => ( + + ))} + + ) : null} + + ) +} + export default meta diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 600201ec02b..bfa5cafc2ff 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -103,7 +103,7 @@ describe('Markup', () => { expect(currentItem).toHaveAttribute('aria-current', 'true') }) - it('expands the path to the current item by default', () => { + it('expands the path to the current item (level 2) by default', () => { const {getByRole} = renderWithTheme( @@ -149,6 +149,49 @@ describe('Markup', () => { expect(item221).toBeVisible() }) + it('expands the path to the current item (level 3) by default', () => { + const {getByRole} = renderWithTheme( + + + Item 1 + + Item 1.1 + + + + Item 2 + + Item 2.1 + + Item 2.2 + + Item 2.2.1 + + + + + Item 3 + + ) + + const item1 = getByRole('treeitem', {name: 'Item 1'}) + const item2 = getByRole('treeitem', {name: 'Item 2'}) + const item22 = getByRole('treeitem', {name: 'Item 2.2'}) + const item221 = getByRole('treeitem', {name: 'Item 2.2.1'}) + + // Item 1 should not be expanded because it is not the parent of the current item + expect(item1).toHaveAttribute('aria-expanded', 'false') + + // Item 2 should be expanded because it is the parent of the current item + expect(item2).toHaveAttribute('aria-expanded', 'true') + + // Item 2.2 should be expanded because it is the current item + expect(item22).toHaveAttribute('aria-expanded', 'true') + + // Item 2.2.1 should be the current item + expect(item221).toHaveAttribute('aria-current', 'true') + }) + it('expands the path to the current item when the current item is changed', () => { function TestTree() { const [current, setCurrent] = React.useState('item1') @@ -941,3 +984,44 @@ describe('Keyboard interactions', () => { }) }) }) + +describe('Controlled state', () => { + it('can be controlled', () => { + function TestTree() { + const [expanded, setExpanded] = React.useState(true) + return ( + + + Parent + + Child + + + + ) + } + + const {getByRole} = renderWithTheme() + + const root = getByRole('tree') + const parent = getByRole('treeitem', {name: 'Parent'}) + const child = getByRole('treeitem', {name: 'Child'}) + + // Parent should be expanded + expect(parent).toHaveAttribute('aria-expanded', 'true') + expect(child).toBeVisible() + + // aria-activedescendant should be set to parent + expect(root).toHaveAttribute('aria-activedescendant', parent.id) + + // Focus tree + root.focus() + + // Press ← to collapse the parent + fireEvent.keyDown(document.activeElement || document.body, {key: 'ArrowLeft'}) + + // Parent should be collapsed + expect(parent).toHaveAttribute('aria-expanded', 'false') + expect(child).not.toBeVisible() + }) +}) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 5e714021ea6..7b41b2076d4 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -3,6 +3,7 @@ import {useSSRSafeId} from '@react-aria/ssr' import React from 'react' import styled from 'styled-components' import Box from '../Box' +import {useControllableState} from '../hooks/useControllableState' import sx, {SxProp} from '../sx' import {Theme} from '../ThemeProvider' import {useActiveDescendant} from './useActiveDescendant' @@ -21,9 +22,11 @@ const RootContext = React.createContext<{ const ItemContext = React.createContext<{ level: number isExpanded: boolean + expandParents: () => void }>({ level: 1, - isExpanded: false + isExpanded: false, + expandParents: () => {} }) // ---------------------------------------------------------------------------- @@ -78,41 +81,60 @@ export type TreeViewItemProps = { children: React.ReactNode current?: boolean defaultExpanded?: boolean + expanded?: boolean + onExpandedChange?: (expanded: boolean) => void onSelect?: (event: React.MouseEvent | KeyboardEvent) => void - onToggle?: (isExpanded: boolean) => void } const Item: React.FC = ({ - current: isCurrent = false, + current: isCurrentItem = false, defaultExpanded = false, + expanded, + onExpandedChange, onSelect, - onToggle, children }) => { const {setActiveDescendant} = React.useContext(RootContext) const itemId = useSSRSafeId() const labelId = useSSRSafeId() const itemRef = React.useRef(null) - const {level} = React.useContext(ItemContext) - const [isExpanded, setIsExpanded] = React.useState(defaultExpanded) + const [isExpanded, setIsExpanded] = useControllableState({ + name: itemId, + defaultValue: defaultExpanded, + value: expanded, + onChange: onExpandedChange + }) + const {level, expandParents} = React.useContext(ItemContext) const {hasSubTree, subTree, childrenWithoutSubTree} = useSubTree(children) // Expand or collapse the subtree const toggle = React.useCallback( (event?: React.MouseEvent) => { - onToggle?.(!isExpanded) setIsExpanded(!isExpanded) event?.stopPropagation() }, - [isExpanded, onToggle] + // setIsExpanded is stable + // eslint-disable-next-line react-hooks/exhaustive-deps + [isExpanded] ) - // Expand item if it is the current item or contains the current item - React.useLayoutEffect(() => { - if (isCurrent || itemRef.current?.querySelector('[aria-current=true]')) { + // Expand all parents of this item including itself + const expandParentsAndSelf = React.useCallback( + () => { + expandParents() setIsExpanded(true) + }, + // setIsExpanded is stable + // eslint-disable-next-line react-hooks/exhaustive-deps + [expandParents] + ) + + // If this item is the current item, expand it and all its parents + React.useLayoutEffect(() => { + if (isCurrentItem) { + expandParentsAndSelf() } - }, [itemRef, isCurrent, subTree]) + }, [isCurrentItem, expandParentsAndSelf]) React.useEffect(() => { const element = itemRef.current @@ -149,7 +171,7 @@ const Item: React.FC = ({ }, [toggle, onSelect, isExpanded]) return ( - +
  • = ({ aria-labelledby={labelId} aria-level={level} aria-expanded={hasSubTree ? isExpanded : undefined} - aria-current={isCurrent ? 'true' : undefined} + aria-current={isCurrentItem ? 'true' : undefined} > { diff --git a/src/__tests__/deprecated/Button.test.tsx b/src/__tests__/deprecated/Button.test.tsx index 7d7cf9f3b74..dd3b02cc2d2 100644 --- a/src/__tests__/deprecated/Button.test.tsx +++ b/src/__tests__/deprecated/Button.test.tsx @@ -15,7 +15,6 @@ import {axe, toHaveNoViolations} from 'jest-axe' expect.extend(toHaveNoViolations) -// eslint-disable-next-line @typescript-eslint/no-empty-function function noop() {} describe('Button', () => { diff --git a/src/hooks/__tests__/useControllableState.test.tsx b/src/hooks/__tests__/useControllableState.test.tsx new file mode 100644 index 00000000000..7d2493737ed --- /dev/null +++ b/src/hooks/__tests__/useControllableState.test.tsx @@ -0,0 +1,83 @@ +import {render} from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import React, {useState} from 'react' +import {useControllableState} from '../useControllableState' + +describe('useControllableState', () => { + test('uncontrolled', async () => { + const {getByTestId} = render() + const input = getByTestId('input') + await userEvent.type(input, 'test') + expect(input).toHaveValue('test') + }) + + test('controlled', async () => { + const {getByTestId} = render() + const input = getByTestId('input') + await userEvent.type(input, 'test') + expect(input).toHaveValue('test') + }) + + test('controlled to uncontrolled', async () => { + const error = jest.spyOn(console, 'error').mockImplementation(() => {}) + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}) + + const {getByTestId} = render() + await userEvent.click(getByTestId('toggle')) + + expect(error).toHaveBeenCalled() + expect(warn).toHaveBeenCalled() + + error.mockRestore() + warn.mockRestore() + }) + + test('uncontrolled to controlled', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}) + + const {getByTestId} = render() + await userEvent.click(getByTestId('toggle')) + + expect(warn).toHaveBeenCalled() + + warn.mockRestore() + }) +}) + +function TextInput({onChange, value: controlledValue}: {onChange?: (value: string) => void; value?: string}) { + const [value, setValue] = useControllableState({ + value: controlledValue, + defaultValue: '', + onChange + }) + + function handleOnChange(event: React.ChangeEvent) { + setValue(event.target.value) + } + + return +} + +function ControlledTextInput() { + const [value, setValue] = useState('') + return +} + +function Toggle({defaultControlled}: {defaultControlled: boolean}) { + const [value, setValue] = useState('') + const [controlled, setControlled] = useState(defaultControlled) + return ( + <> + + + + ) +} diff --git a/src/hooks/useControllableState.ts b/src/hooks/useControllableState.ts new file mode 100644 index 00000000000..90efbee4fbc --- /dev/null +++ b/src/hooks/useControllableState.ts @@ -0,0 +1,119 @@ +import React from 'react' +declare let __DEV__: boolean + +type ControllableStateOptions = { + /** + * A unique name for the state value. + */ + name?: string + /** + * The default value used for the state. This will be + * the fallback value used if `value` is not defined. + * */ + defaultValue: T + /** + * A controlled value. Omitting this means that the state is uncontrolled. + */ + value?: T + /** + * An optional function that is called when the value of the state changes. + * This is useful for communicating to parents of controlled components + * that the value is requesting to be changed. + */ + onChange?: (value: T) => void +} + +/** + * This custom hook simplifies the behavior of a component if it has state that + * can be both controlled and uncontrolled. It functions identical to a + * useState() hook and provides [state, setState] for you to use. You can use + * the `onChange` argument to allow updates to the `state` to be communicated to + * owners of controlled components. + * + * Note: This hook will warn if a component is switching from controlled to + * uncontrolled, or vice-versa. + */ +export function useControllableState({ + name = 'custom', + defaultValue, + value, + onChange +}: ControllableStateOptions): [T, React.Dispatch>] { + const [state, internalSetState] = React.useState(value ?? defaultValue) + const controlled = React.useRef(null) + const stableOnChange = React.useRef(onChange) + + React.useEffect(() => { + stableOnChange.current = onChange + }) + + if (controlled.current === null) { + controlled.current = value !== undefined + } + + const setState = React.useCallback( + (stateOrUpdater: T | ((prevState: T) => T)) => { + const value = + typeof stateOrUpdater === 'function' + ? // @ts-ignore stateOrUpdater is a function + stateOrUpdater(state) + : stateOrUpdater + + if (controlled.current === false) { + internalSetState(value) + } + + stableOnChange.current?.(value) + }, + [state] + ) + + React.useEffect(() => { + const controlledValue = value !== undefined + + // Uncontrolled -> Controlled + // If the component prop is uncontrolled, the prop value should be undefined + if (controlled.current === false && controlledValue) { + warn( + 'A component is changing an uncontrolled %s component to be controlled. ' + + 'This is likely caused by the value changing to a defined value ' + + 'from undefined. Decide between using a controlled or uncontrolled ' + + 'value for the lifetime of the component. ' + + 'More info: https://reactjs.org/link/controlled-components', + name + ) + } + + // Controlled -> Uncontrolled + // If the component prop is controlled, the prop value should be defined + if (controlled.current === true && !controlledValue) { + warn( + 'A component is changing a controlled %s component to be uncontrolled. ' + + 'This is likely caused by the value changing to an undefined value ' + + 'from a defined one. Decide between using a controlled or ' + + 'uncontrolled value for the lifetime of the component. ' + + 'More info: https://reactjs.org/link/controlled-components', + name + ) + } + }, [name, value]) + + if (controlled.current === true) { + return [value as T, setState] + } + + return [state, setState] +} + +/** Warn when running in a development environment */ +const warn = __DEV__ + ? // eslint-disable-next-line @typescript-eslint/no-explicit-any + function warn(format: string, ...args: any[]) { + let index = 0 + const message = format.replace(/%s/g, () => { + return args[index++] + }) + // eslint-disable-next-line no-console + console.warn(`Warning: ${message}`) + } + : function emptyFunction() {} diff --git a/src/hooks/useMedia.tsx b/src/hooks/useMedia.tsx index c18bf6b6282..ce6ea1df47b 100644 --- a/src/hooks/useMedia.tsx +++ b/src/hooks/useMedia.tsx @@ -61,7 +61,6 @@ export function useMedia(mediaQueryString: string, defaultState?: boolean) { const mediaQueryList = window.matchMedia(mediaQueryString) // Support fallback to `addListener` for broader browser support - // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore this is not present in Safari <14 // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition if (mediaQueryList.addEventListener) { @@ -74,7 +73,6 @@ export function useMedia(mediaQueryString: string, defaultState?: boolean) { setMatches(mediaQueryList.matches) return () => { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore this is not present in Safari <14 // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition if (mediaQueryList.addEventListener) { diff --git a/src/hooks/useResizeObserver.ts b/src/hooks/useResizeObserver.ts index 34e5135656d..4ed92e1de44 100644 --- a/src/hooks/useResizeObserver.ts +++ b/src/hooks/useResizeObserver.ts @@ -11,7 +11,6 @@ export interface ResizeObserverEntry { export function useResizeObserver(callback: ResizeObserverCallback, target?: RefObject) { useLayoutEffect(() => { const targetEl = target && 'current' in target ? target.current : document.documentElement - // eslint-disable-next-line @typescript-eslint/no-empty-function if (!targetEl) return () => {} const observer = new window.ResizeObserver(entries => callback(entries)) observer.observe(targetEl) diff --git a/src/polyfills/eventListenerSignal.ts b/src/polyfills/eventListenerSignal.ts index 94276635209..f2f4d63c7ee 100644 --- a/src/polyfills/eventListenerSignal.ts +++ b/src/polyfills/eventListenerSignal.ts @@ -10,7 +10,6 @@ removed. */ let signalSupported = false -// eslint-disable-next-line @typescript-eslint/no-empty-function function noop() {} try { const options = Object.create( diff --git a/src/utils/deprecate.tsx b/src/utils/deprecate.tsx index 28d9dd25a77..49c29205fa0 100644 --- a/src/utils/deprecate.tsx +++ b/src/utils/deprecate.tsx @@ -3,7 +3,6 @@ declare let __DEV__: boolean type DeprecationType = {name: string; message: string; version: string} -// eslint-disable-next-line @typescript-eslint/no-empty-function const noop = () => {} // eslint-disable-next-line import/no-mutable-exports let deprecate: ({name, message, version}: DeprecationType) => void | (() => void) = noop