TreeView: Support async behavior #2388
Conversation
🦋 Changeset detectedLatest commit: 172987e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
src/TreeView/TreeView.stories.tsx
Outdated
| onExpandedChange={async isExpanded => { | ||
| if (asyncItems.length === 0 && isExpanded) { | ||
| // Show loading indicator after a short delay | ||
| const timeout = setTimeout(() => setIsLoading(true), 500) |
There was a problem hiding this comment.
🙋 Is showing the loading item after a short delay an accessible pattern? cc @ericwbailey
There was a problem hiding this comment.
Context for @ericwbailey: we do this to avoid showing loading indicators before a user might need them. When users see a lot of loading indicators, I think it makes the UI seem slower than it actually is.
There was a problem hiding this comment.
I think this should be okay.
| const LoadingItem: React.FC = () => { | ||
| return ( | ||
| // TODO: What aria attributes do we need to add here? | ||
| <Item> |
There was a problem hiding this comment.
🙋 What aria props should we add to the LoadingItem? cc @ericwbailey
There was a problem hiding this comment.
I'm in the process of testing this out, but I believe we'll need a combination of aria-hidden (to hide decorative nodes), aria-busy (to communicate loading state), and aria-live (to announce state change).
| <TreeView.SubTree> | ||
| {isLoading ? <TreeView.LoadingItem /> : null} | ||
| {error ? ( | ||
| <ConfirmationDialog |
There was a problem hiding this comment.
🙋 Is ConfirmationDialog the right kind of dialog for error messages? cc @ericwbailey
colebemis
requested review from
ericwbailey and
mperrotti
and removed request for
siddharthkp
| ) | ||
| } | ||
|
|
||
| LoadingItem.displayName = 'TreeView.LoadingItem' |
There was a problem hiding this comment.
I like this. I'm going to start doing this instead of keeping the name of the const.
src/TreeView/TreeView.stories.tsx
Outdated
| onExpandedChange={async isExpanded => { | ||
| if (asyncItems.length === 0 && isExpanded) { | ||
| // Show loading indicator after a short delay | ||
| const timeout = setTimeout(() => setIsLoading(true), 500) |
There was a problem hiding this comment.
500ms felt a little too long to see expanded content. Maybe we could try 250-300ms?
This is non-blocking feedback and just one person's opinion.
There was a problem hiding this comment.
Agreed. I'll reduce it to 300ms
| @@ -417,7 +434,7 @@ const LeadingVisual: React.FC<TreeViewVisualProps> = props => { | |||
| const children = typeof props.children === 'function' ? props.children({isExpanded}) : props.children | |||
| return ( | |||
| <Slot name="LeadingVisual"> | |||
| <Box sx={{color: 'fg.muted'}}>{children}</Box> | |||
| <Box sx={{display: 'flex', color: 'fg.muted'}}>{children}</Box> | |||
There was a problem hiding this comment.
What do we need flexbox for? Is it just a vertical alignment thing? Or are we actually laying out multiple children?
There was a problem hiding this comment.
When you render an SVG (Octicon) in the leading visual slot, it adds unwanted extra space unless the container is a flex container.
There was a problem hiding this comment.
Ok, I figured it was something like that. I've dealt with that before, and I don't understand why it works 🙈
I think it also works for grid
Summary
TreeView.LoadingItemcomponent to render while loading TreeView items.Demos
Loading items (success)
👉 Try it out
CleanShot.2022-09-30.at.11.22.07.mp4
Loading items (error)
👉 Try it out
CleanShot.2022-09-30.at.10.49.04.mp4
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.