v3 TreeView #6020

LFDanLu · 2024-03-07T01:36:18Z

Closes

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Test the TreeView stories. See https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=733 and
https://www.chromatic.com/build?appId=651b502f6b74abaae4ee734c&number=24 for the chromatic stories

🧢 Your Project:

RSP

…conversion

…op considerations

default pointer styling, focus styling when focus is withing the row, clicking on the chevron should move focus into tree, checkbox now appears when single selection

conditions to macros must have "is" as a prefix?

…m rainbow for macros

…on menu icon

… run still

…erronously overwritten icon slot propogated styles from TreeView row were making it to the ActionMenu button. Preventing this from happening by making ActionButton clearSlots and only accept text slot props. To preserve ActionGroup classname for icon, moved it to ActionButton directly

rspbot · 2024-04-18T23:28:13Z

Build successful! 🎉

rspbot · 2024-04-22T18:18:48Z

Build successful! 🎉

snowystinger · 2024-04-22T21:45:27Z

packages/@react-spectrum/actiongroup/src/ActionGroup.tsx

            }
          }}>
          <ActionButton
            ref={ref}
+            // @ts-ignore (private)
+            hideButtonText={hideButtonText}


yeaaaah pretty annoying that I had to do this, open to any alternative ideas

snowystinger · 2024-04-22T23:39:03Z

packages/@react-spectrum/tree/package.json

+    "@react-aria/tree": "3.0.0-alpha.1",
+    "@react-aria/utils": "^3.23.2",
+    "@react-spectrum/checkbox": "^3.9.4",
+    "@react-spectrum/style-macro-s1": "^3.5.3",


is that really the right version?

oh hm, well it is the version that is set on the current style macro package but I can see your point. I'll to drop it down to 1.0.0-alpha.1 I guess since that package is private

snowystinger · 2024-04-22T23:51:57Z

packages/@react-spectrum/tree/src/TreeView.tsx

+    default: 'default',
+    isLink: 'pointer'
+  },
+  // TODO: not sure where to get the equivalent colors here, for instance isHovered is spectrum 600 with 10% opacity but I don't think that exists in the theme


still true?
can we add it?

yep, still true from what I can tell, I'd be happy to add it but I'm just not sure what our process for that is for v3. Do I just add whatever I need to the spectrum-theme?

yeah, i think that'd be fine, so long as it's not a one off thing, if it is, then maybe the colors should be revisited

kk, I'll add that as one of the post alpha followups

snowystinger · 2024-04-23T00:45:15Z

packages/@react-spectrum/tree/src/TreeView.tsx

+  }
+
+  return (
+    // TODO right now all the tree rows have the various data attributes applied on their dom nodes, should they? Doesn't feel very useful


I don't think it matters if they are just leftover from RAC

snowystinger · 2024-04-23T00:48:36Z

packages/@react-spectrum/tree/src/TreeView.tsx

+  transition: '[transform ease var(--spectrum-global-animation-duration-100)]'
+});
+
+function ExpandableRowChevronMacros(props: ExpandableRowChevronProps) {


why include "Macros" in the name? is it different than the style macros or is there a non-macros one? otherwise I think it's unnecessary. i know it's internal only, but i started looking for things it might mean to warrant it

I don't actually remember why I named it this haha, I think it was just to set it apart from the TableView's ExpandableRowChevron but I'll just rename this

snowystinger · 2024-04-23T00:54:49Z

packages/react-aria-components/src/Tree.tsx

+  // TODO: api discussion, how do we feel about the below? This is so we can still style the row as grey when a child element within is focused
+  // Maybe should have this for the other collection item render props
+  // Whether the tree item's children have keyboard focus.
+  isFocusVisibleWithin: boolean


makes sense to me
i'm fine with the name as well

I'm surprised we don't have this on Table RAC, meanwhile, TableView has this kind of styling which means we'll need this in Table as well

snowystinger · 2024-04-23T00:57:06Z

scripts/setupTests.js

@@ -91,5 +92,7 @@ expect.extend({
  }
 });

+configure({asyncUtilTimeout: 400});


does this still happen now after devon's work?

…_treeview

rspbot · 2024-04-23T22:25:40Z

Build successful! 🎉

ktabors

A couple non-blocking questions. Looks great!

packages/@react-spectrum/tree/chromatic/TreeView.chromatic.tsx

packages/@react-spectrum/tree/src/TreeView.tsx

ktabors · 2024-04-24T04:57:37Z

packages/@react-spectrum/tree/src/TreeView.tsx

+                UNSAFE_style={{paddingInlineEnd: '0px'}}
+                slot="selection" />
+            )}
+            <div style={{gridArea: 'level-padding', marginInlineEnd: `calc(${level - 1} * var(--spectrum-global-dimension-size-200))`}} />


Is there a reason we're using style instead of a style() macro in className?

Ah good point, I can add that in a followup

rspbot · 2024-04-24T20:50:28Z

Build successful! 🎉

rspbot · 2024-04-24T20:50:29Z

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@react-spectrum/tree

Item

-Item<T> {
-  props: ItemProps<T>
-  returnVal: undefined
-}
+

Section

-Section<T> {
-  props: SectionProps<T>
-  returnVal: undefined
-}
+

Tree

-Tree<T extends {}> {
 
-}

TreeViewItem

-
+TreeViewItem {
+  TreeViewItem?: boolean
+}

TreeView

-
+TreeView<T extends {}> {
+  children?: ReactNode | ({}) => ReactNode
+  onAction?: (Key) => void
+  renderEmptyState?: () => JSX.Element
+}

SpectrumTreeViewProps

-
+SpectrumTreeViewProps<T> {
+  children?: ReactNode | (T) => ReactNode
+  onAction?: (Key) => void
+  renderEmptyState?: () => JSX.Element
+}

SpectrumTreeViewItemProps

-
+SpectrumTreeViewItemProps {
+  children: ReactNode
+  hasChildItems?: boolean
+}

devongovett · 2024-04-29T16:27:47Z

packages/@react-spectrum/tree/src/TreeView.tsx

+  forcedColorAdjust: 'none',
+
+  boxShadow: {
+    isFocusVisible: '[inset 2px 0 0 0 var(--spectrum-alias-focus-color), inset -2px 0 0 0 var(--spectrum-alias-focus-color), inset 0 -2px 0 0 var(--spectrum-alias-focus-color), inset 0 2px 0 0 var(--spectrum-alias-focus-color)]',


probably could use outline for the focus ring like we are in s2. might simplify this a bit.

Sure, added it to the followup ticket to try

devongovett · 2024-04-29T16:29:33Z

packages/@react-spectrum/tree/src/TreeView.tsx

+      } else {
+        content.push(node);
+      }
+    });


We generally try to avoid this kind of thing with the new collections API since someone could wrap TreeViewItem in their own custom component. Should we have a TreeItemContent component in Spectrum like we do in RAC?

Definitely could go that route, I can play around with that

LFDanLu added 12 commits March 7, 2024 15:43

scaffolding

a10cc8e

more style progress

870767c

add box shadow border styles

d392e1b

add disabled styles and behavior

4eab1d3

get rid of psudo element in favor of real element in prep for macros …

383f2c3

…conversion

get macro styles to work for tree

70e9327

remove old version in favor of macros

a3ec22e

add dynamic support

a939309

add icons support, types, ref support and misc cleanup

1c987d5

fix lint and rename Tree file

3a415eb

add resizing story and fix text truncation/border cut off/addition pr…

9b582b3

…op considerations

fix wiggle

61e3d0f

LFDanLu force-pushed the S1_treeview branch from ad3d526 to 61e3d0f Compare March 7, 2024 23:44

LFDanLu added 17 commits March 7, 2024 17:25

add more stories, fix chevron grid sizing, add empty state centering

7d6bbf7

progress in fixing build issue

21ea542

fix bugs from testing

4b6f40b

default pointer styling, focus styling when focus is withing the row, clicking on the chevron should move focus into tree, checkbox now appears when single selection

fix pointer

57c5277

conditions to macros must have "is" as a prefix?

tentative announcement for grid list rows with links/actions

8b6c889

updating HCM, RTL and increasing row width for action button focus ring

5e7a086

weirdness with styles bleeding into the action menu, may need fix fro…

b45ea90

…m rainbow for macros

brainstorming some way to fix tree icon styles from making it to acti…

98c32e0

…on menu icon

adding temp test setup and babel config update for macros, tests dont…

b336725

… run still

chevron rotate animation

bfe77b5

bump babel so tests with macros can run

8314c41

Merge branch 'main' of github.com:adobe/react-spectrum into S1_treeview

5105862

update treeview tests to match spectrum implementation

3e289b4

wrap up tests and clean up package json

accf926

test skipping the tree tests to see if the 16 and 17 tests pass

51655dc

unskip tree tests, doesnt look like those are the problem

bb06387

LFDanLu added the release label Apr 17, 2024

LFDanLu added 2 commits April 18, 2024 14:15

Merge branch 'main' of github.com:adobe/react-spectrum into S1_treeview

e896bb9

get rid of things added to pass the build now that we are using SWC

85d0f58

LFDanLu dismissed reidbarber’s stale review via 85d0f58 April 18, 2024 21:22

still need importAttributes for docs transformer?

6b6492f

Merge branch 'main' of github.com:adobe/react-spectrum into S1_treeview

896ad4c

reidbarber previously approved these changes Apr 22, 2024

View reviewed changes

snowystinger reviewed Apr 23, 2024

View reviewed changes

Address review comments

c0e38b8

LFDanLu dismissed reidbarber’s stale review via c0e38b8 April 23, 2024 21:58

LFDanLu added 4 commits April 23, 2024 14:59

Merge branch 'main' into S1_treeview

7213dfe

typo

6dd23aa

Merge branch 'S1_treeview' of github.com:adobe/react-spectrum into S1…

77ac56a

…_treeview

fix import

503c6e3

ktabors previously approved these changes Apr 24, 2024

View reviewed changes

snowystinger previously approved these changes Apr 24, 2024

View reviewed changes

Merge branch 'main' of github.com:adobe/react-spectrum into S1_treeview

3882ad3

LFDanLu dismissed stale reviews from snowystinger and ktabors via 3882ad3 April 24, 2024 20:35

ktabors approved these changes Apr 24, 2024

View reviewed changes

reidbarber approved these changes Apr 24, 2024

View reviewed changes

LFDanLu merged commit f3cbe03 into main Apr 25, 2024
25 checks passed

LFDanLu deleted the S1_treeview branch April 25, 2024 21:42

devongovett reviewed Apr 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v3 TreeView #6020

v3 TreeView #6020

LFDanLu commented Mar 7, 2024 •

edited

rspbot commented Apr 18, 2024

rspbot commented Apr 22, 2024

snowystinger Apr 22, 2024

LFDanLu Apr 23, 2024

snowystinger Apr 22, 2024

LFDanLu Apr 23, 2024

snowystinger Apr 22, 2024

LFDanLu Apr 23, 2024

snowystinger Apr 24, 2024

LFDanLu Apr 24, 2024

snowystinger Apr 23, 2024

snowystinger Apr 23, 2024

LFDanLu Apr 23, 2024

snowystinger Apr 23, 2024

snowystinger Apr 23, 2024

rspbot commented Apr 23, 2024

ktabors left a comment

ktabors Apr 24, 2024

LFDanLu Apr 24, 2024

rspbot commented Apr 24, 2024

rspbot commented Apr 24, 2024

devongovett Apr 29, 2024

LFDanLu Apr 29, 2024

devongovett Apr 29, 2024

LFDanLu Apr 29, 2024

v3 TreeView #6020

v3 TreeView #6020

Conversation

LFDanLu commented Mar 7, 2024 • edited

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Apr 18, 2024

rspbot commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented Apr 23, 2024

ktabors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented Apr 24, 2024

rspbot commented Apr 24, 2024

@react-spectrum/tree

Item

Section

Tree

TreeViewItem

TreeView

SpectrumTreeViewProps

SpectrumTreeViewItemProps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LFDanLu commented Mar 7, 2024 •

edited