From 4fdf6b4df2abff8b5ecdf73ffe245885ec65ae67 Mon Sep 17 00:00:00 2001 From: "Amanda G. Brown" Date: Thu, 15 Dec 2022 14:41:02 -0500 Subject: [PATCH 1/9] fix(TreeView): remove "folder empty" announcement/logic Co-authored-by: Josh Black --- .gitignore | 1 + src/TreeView/TreeView.tsx | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 0812cbae758..a044e6566fa 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,4 @@ public/ stats.html .env storybook-static +.tool-versions diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 21f2660f3b2..8e93cfc8053 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -75,7 +75,7 @@ const UlBox = styled.ul` * We define styles for the tree items at the root level of the tree * to avoid recomputing the styles for each item when the tree updates. * We're sacraficing maintainability for performance because TreeView - * needs to be performant enough to handle large trees (thousands of items). + * needs to be performant enough to handle large trees (thousands of items). * * This is intended to be a temporary solution until we can improve the * performance of our styling patterns. @@ -402,6 +402,7 @@ const Item = React.forwardRef( aria-labelledby={labelId} aria-describedby={`${leadingVisualId} ${trailingVisualId}`} aria-level={level} + // aria-expanded={isSubTreeEmpty ? undefined : isExpanded} aria-expanded={isSubTreeEmpty ? undefined : isExpanded} aria-current={isCurrentItem ? 'true' : undefined} onKeyDown={handleKeyDown} @@ -503,6 +504,13 @@ const SubTree: React.FC = ({count, state, children}) => { const {safeSetTimeout} = useSafeTimeout() const loadingItemRef = React.useRef(null) const ref = React.useRef(null) + const [isPending, setPending] = React.useState(state === 'loading') + + React.useEffect(() => { + if (state === 'loading') { + setPending(true) + } + }, [state]) React.useEffect(() => { // If `state` is undefined, we're working in a synchronous context and need @@ -520,7 +528,7 @@ const SubTree: React.FC = ({count, state, children}) => { // Announce when content has loaded React.useEffect(() => { - if (state === 'done') { + if (isPending && state === 'done') { const parentItem = document.getElementById(itemId) if (!parentItem) return @@ -535,8 +543,10 @@ const SubTree: React.FC = ({count, state, children}) => { announceUpdate(`${parentName} is empty`) } }) + + setPending(false) } - }, [state, itemId, announceUpdate, safeSetTimeout]) + }, [state, itemId, announceUpdate, safeSetTimeout, isPending]) // Manage loading indicator state React.useEffect(() => { From 72813e476a25966432435c6b5eeb7d5af5644a00 Mon Sep 17 00:00:00 2001 From: "Amanda G. Brown" Date: Mon, 19 Dec 2022 10:49:16 -0500 Subject: [PATCH 2/9] test(TreeView): added nested subtree to TreeView test --- src/TreeView/TreeView.test.tsx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 03c77138768..2c27240ec19 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -1070,7 +1070,14 @@ describe('Asyncronous loading', () => { Parent - Child + Child Item + + Child Subtree + + Child 1 + Child 2 + + @@ -1093,6 +1100,7 @@ describe('Asyncronous loading', () => { }) // Live region should be updated + expect(liveRegion).not.toHaveTextContent('Child 2 is empty') expect(liveRegion).toHaveTextContent('Parent content loaded') }) From 2c2763b09fc08205f4d2a473e9b715fce7265a9a Mon Sep 17 00:00:00 2001 From: "Amanda G. Brown" Date: Mon, 19 Dec 2022 11:16:18 -0500 Subject: [PATCH 3/9] chore: update storybook and add changeset --- .changeset/rotten-hairs-impress.md | 5 + src/TreeView/TreeView.features.stories.tsx | 126 +++++++++++++++++---- 2 files changed, 107 insertions(+), 24 deletions(-) create mode 100644 .changeset/rotten-hairs-impress.md diff --git a/.changeset/rotten-hairs-impress.md b/.changeset/rotten-hairs-impress.md new file mode 100644 index 00000000000..6368af3c093 --- /dev/null +++ b/.changeset/rotten-hairs-impress.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +aria status description is now accurate diff --git a/src/TreeView/TreeView.features.stories.tsx b/src/TreeView/TreeView.features.stories.tsx index a2ce51614ae..4f9c345daa1 100644 --- a/src/TreeView/TreeView.features.stories.tsx +++ b/src/TreeView/TreeView.features.stories.tsx @@ -17,8 +17,8 @@ const meta: Meta = { ) - }, - ], + } + ] } export const Files: Story = () => ( @@ -197,9 +197,9 @@ function expandAll(tree: TreeItem[]): TreeItem[] { return tree.map(item => ({ data: { ...item.data, - expanded: true, + expanded: true }, - children: expandAll(item.children), + children: expandAll(item.children) })) } @@ -207,9 +207,9 @@ function collapseAll(tree: TreeItem[]): TreeItem[] { return tree.map(item => ({ data: { ...item.data, - expanded: false, + expanded: false }, - children: collapseAll(item.children), + children: collapseAll(item.children) })) } @@ -220,13 +220,13 @@ function setExpanded(tree: TreeItem[], path: string[], expanded: boolean): TreeI ...item, data: { ...item.data, - expanded, - }, + expanded + } } } else if (item.data.name === path[0]) { return { ...item, - children: setExpanded(item.children, path.slice(1), expanded), + children: setExpanded(item.children, path.slice(1), expanded) } } else { return item @@ -239,27 +239,27 @@ const CurrentPathContext = React.createContext<{ setCurrentPath: React.Dispatch> }>({ currentPath: [], - setCurrentPath: () => {}, + setCurrentPath: () => {} }) const intialTree: TreeItem[] = Array.from({length: 5}).map((_, i) => ({ data: { name: `Item ${i}`, - expanded: false, + expanded: false }, children: Array.from({length: 5}).map((_, j) => ({ data: { name: `Item ${i}.${j}`, - expanded: false, + expanded: false }, children: Array.from({length: 5}).map((_, k) => ({ data: { name: `Item ${i}.${j}.${k}`, - expanded: false, + expanded: false }, - children: [], - })), - })), + children: [] + })) + })) })) export const Controlled: Story = () => { @@ -293,7 +293,7 @@ export const Controlled: Story = () => { function TreeItem({ item, path, - onExpandedChange, + onExpandedChange }: { item: TreeItem path: string[] @@ -396,7 +396,7 @@ export const AsyncSuccess: Story = args => { } AsyncSuccess.args = { - responseTime: 2000, + responseTime: 2000 } export const AsyncWithCount: Story = args => { @@ -462,13 +462,13 @@ export const AsyncWithCount: Story = args => { AsyncWithCount.args = { responseTime: 2000, - count: 3, + count: 3 } AsyncWithCount.argTypes = { count: { - type: 'number', - }, + type: 'number' + } } async function alwaysFails(responseTime: number) { @@ -563,7 +563,7 @@ export const AsyncError: Story = args => { } AsyncError.args = { - responseTime: 2000, + responseTime: 2000 } export const EmptyDirectories: Story = () => { @@ -609,6 +609,84 @@ export const EmptyDirectories: Story = () => { ) } +export const NestedTrees: Story = () => { + const [isLoading, setIsLoading] = React.useState(false) + const [asyncItems, setAsyncItems] = React.useState([]) + + let state: SubTreeState = 'initial' + + if (isLoading) { + state = 'loading' + } else if (asyncItems.length > 0) { + state = 'done' + } + + return ( + + ) +} + export const NestedScrollContainer: Story = () => { return ( @@ -667,7 +745,7 @@ export const StressTest: Story = () => { } StressTest.parameters = { - chromatic: {disableSnapshot: true}, + chromatic: {disableSnapshot: true} } export const ContainIntrinsicSize: Story = () => { @@ -696,7 +774,7 @@ export const ContainIntrinsicSize: Story = () => { } ContainIntrinsicSize.parameters = { - chromatic: {disableSnapshot: true}, + chromatic: {disableSnapshot: true} } export default meta From 93e7c920e0f530cabd87afb811fb8c6a924722d1 Mon Sep 17 00:00:00 2001 From: "Amanda G. Brown" Date: Mon, 19 Dec 2022 11:54:34 -0500 Subject: [PATCH 4/9] chore: fix lint --- src/TreeView/TreeView.features.stories.tsx | 48 +++++++++++----------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/TreeView/TreeView.features.stories.tsx b/src/TreeView/TreeView.features.stories.tsx index 4f9c345daa1..bf19cdd7b0f 100644 --- a/src/TreeView/TreeView.features.stories.tsx +++ b/src/TreeView/TreeView.features.stories.tsx @@ -17,8 +17,8 @@ const meta: Meta = { ) - } - ] + }, + ], } export const Files: Story = () => ( @@ -197,9 +197,9 @@ function expandAll(tree: TreeItem[]): TreeItem[] { return tree.map(item => ({ data: { ...item.data, - expanded: true + expanded: true, }, - children: expandAll(item.children) + children: expandAll(item.children), })) } @@ -207,9 +207,9 @@ function collapseAll(tree: TreeItem[]): TreeItem[] { return tree.map(item => ({ data: { ...item.data, - expanded: false + expanded: false, }, - children: collapseAll(item.children) + children: collapseAll(item.children), })) } @@ -220,13 +220,13 @@ function setExpanded(tree: TreeItem[], path: string[], expanded: boolean): TreeI ...item, data: { ...item.data, - expanded - } + expanded, + }, } } else if (item.data.name === path[0]) { return { ...item, - children: setExpanded(item.children, path.slice(1), expanded) + children: setExpanded(item.children, path.slice(1), expanded), } } else { return item @@ -239,27 +239,27 @@ const CurrentPathContext = React.createContext<{ setCurrentPath: React.Dispatch> }>({ currentPath: [], - setCurrentPath: () => {} + setCurrentPath: () => {}, }) const intialTree: TreeItem[] = Array.from({length: 5}).map((_, i) => ({ data: { name: `Item ${i}`, - expanded: false + expanded: false, }, children: Array.from({length: 5}).map((_, j) => ({ data: { name: `Item ${i}.${j}`, - expanded: false + expanded: false, }, children: Array.from({length: 5}).map((_, k) => ({ data: { name: `Item ${i}.${j}.${k}`, - expanded: false + expanded: false, }, - children: [] - })) - })) + children: [], + })), + })), })) export const Controlled: Story = () => { @@ -293,7 +293,7 @@ export const Controlled: Story = () => { function TreeItem({ item, path, - onExpandedChange + onExpandedChange, }: { item: TreeItem path: string[] @@ -396,7 +396,7 @@ export const AsyncSuccess: Story = args => { } AsyncSuccess.args = { - responseTime: 2000 + responseTime: 2000, } export const AsyncWithCount: Story = args => { @@ -462,13 +462,13 @@ export const AsyncWithCount: Story = args => { AsyncWithCount.args = { responseTime: 2000, - count: 3 + count: 3, } AsyncWithCount.argTypes = { count: { - type: 'number' - } + type: 'number', + }, } async function alwaysFails(responseTime: number) { @@ -563,7 +563,7 @@ export const AsyncError: Story = args => { } AsyncError.args = { - responseTime: 2000 + responseTime: 2000, } export const EmptyDirectories: Story = () => { @@ -745,7 +745,7 @@ export const StressTest: Story = () => { } StressTest.parameters = { - chromatic: {disableSnapshot: true} + chromatic: {disableSnapshot: true}, } export const ContainIntrinsicSize: Story = () => { @@ -774,7 +774,7 @@ export const ContainIntrinsicSize: Story = () => { } ContainIntrinsicSize.parameters = { - chromatic: {disableSnapshot: true} + chromatic: {disableSnapshot: true}, } export default meta From c0678574b48a1719ec9ed7f9f96dcb35d5f2dbb6 Mon Sep 17 00:00:00 2001 From: Amanda Brown Date: Thu, 29 Dec 2022 11:57:22 -0500 Subject: [PATCH 5/9] Update src/TreeView/TreeView.tsx Co-authored-by: Cole Bemis --- src/TreeView/TreeView.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 62d53f581a1..af973ca9473 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -402,7 +402,6 @@ const Item = React.forwardRef( aria-labelledby={labelId} aria-describedby={`${leadingVisualId} ${trailingVisualId}`} aria-level={level} - // aria-expanded={isSubTreeEmpty ? undefined : isExpanded} aria-expanded={isSubTreeEmpty ? undefined : isExpanded} aria-current={isCurrentItem ? 'true' : undefined} onKeyDown={handleKeyDown} From f02af4fe0ce3b1179de838928046ade79f536112 Mon Sep 17 00:00:00 2001 From: Amanda Brown Date: Thu, 29 Dec 2022 11:57:54 -0500 Subject: [PATCH 6/9] Update .changeset/rotten-hairs-impress.md Co-authored-by: Cole Bemis --- .changeset/rotten-hairs-impress.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/rotten-hairs-impress.md b/.changeset/rotten-hairs-impress.md index 6368af3c093..1ee727e88c1 100644 --- a/.changeset/rotten-hairs-impress.md +++ b/.changeset/rotten-hairs-impress.md @@ -2,4 +2,4 @@ '@primer/react': minor --- -aria status description is now accurate +TreeView: aria status description is now accurate From e10d724e7cf8c0ac1b329e4a7ed7bb529ce0211a Mon Sep 17 00:00:00 2001 From: Amanda Brown Date: Tue, 24 Jan 2023 12:32:29 -0500 Subject: [PATCH 7/9] Update .changeset/rotten-hairs-impress.md Co-authored-by: Cole Bemis --- .changeset/rotten-hairs-impress.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/rotten-hairs-impress.md b/.changeset/rotten-hairs-impress.md index 1ee727e88c1..190b02f671b 100644 --- a/.changeset/rotten-hairs-impress.md +++ b/.changeset/rotten-hairs-impress.md @@ -1,5 +1,5 @@ --- -'@primer/react': minor +'@primer/react': patch --- TreeView: aria status description is now accurate From 8ba6ece108dabeaf3f4e956fb05978cd5489ee12 Mon Sep 17 00:00:00 2001 From: Amanda Brown Date: Tue, 24 Jan 2023 12:43:09 -0500 Subject: [PATCH 8/9] chore: add comment explaining use of isPending --- src/TreeView/TreeView.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index af973ca9473..1ee9e1ba3b7 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -530,7 +530,9 @@ const SubTree: React.FC = ({count, state, children}) => { } }, [state, isSubTreeEmpty, setIsSubTreeEmpty, children]) - // Announce when content has loaded + // If a consumer sets state="done" without having a previous state (like `loading`), + // then it would announce on the first render. Using isPending is to only + // announce being "loaded" when the state has changed from `loading` --> `done`. React.useEffect(() => { if (isPending && state === 'done') { const parentItem = document.getElementById(itemId) From 84825f0ae62740ae46191e5c90841128b5fa21e3 Mon Sep 17 00:00:00 2001 From: "Amanda G. Brown" Date: Tue, 24 Jan 2023 12:51:43 -0500 Subject: [PATCH 9/9] chore: fixed formatting --- src/TreeView/TreeView.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 1ee9e1ba3b7..0e44e0ff776 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -530,9 +530,9 @@ const SubTree: React.FC = ({count, state, children}) => { } }, [state, isSubTreeEmpty, setIsSubTreeEmpty, children]) - // If a consumer sets state="done" without having a previous state (like `loading`), - // then it would announce on the first render. Using isPending is to only - // announce being "loaded" when the state has changed from `loading` --> `done`. + // If a consumer sets state="done" without having a previous state (like `loading`), + // then it would announce on the first render. Using isPending is to only + // announce being "loaded" when the state has changed from `loading` --> `done`. React.useEffect(() => { if (isPending && state === 'done') { const parentItem = document.getElementById(itemId)