Improve handling of ListView disabled rows, and add storybook controls by devongovett · Pull Request #3123 · adobe/react-spectrum

devongovett · 2022-05-11T23:48:39Z

Reorganized the storybook for ListView to use the controls addon, and organize the stories into categories. Now mostly the stories are about data differences, and controls are used for props that could apply to any story.

Also working on some updates to the selection/navigation behavior for ListView. The new selection stories have an example with hierarchical data and breadcrumbs, which illustrates the behavior better. When an item is disabled, only selection is disabled, and not actions.

To do

Verify the behavior/styles
Tests

…tion

Bug found via storybook controls

adobe-bot · 2022-05-12T00:06:50Z

Build successful! 🎉

…rum into listview-disabled-rows

adobe-bot · 2022-05-12T20:17:55Z

Build successful! 🎉

adobe-bot · 2022-05-16T23:54:52Z

Build successful! 🎉

…disabled-rows # Conflicts: # package.json # packages/@react-spectrum/list/src/ListView.tsx # packages/@react-spectrum/list/src/ListViewItem.tsx # packages/@react-spectrum/list/stories/ListView.stories.tsx # yarn.lock

…disabled-rows # Conflicts: # packages/@react-spectrum/list/stories/ListView.stories.tsx

adobe-bot · 2022-05-18T23:06:14Z

Build successful! 🎉

snowystinger

Problem for later probably, but in https://reactspectrum.blob.core.windows.net/reactspectrum/6db0e0700bb199c3577b53c1ba64fe1719512ee7/storybook/index.html?path=/story/listview-selection--onaction-disable-folder-selection
if I keyboard navigate into the first disabled folder, then shift tab to the breadcrumbs and go up a level, then tab back into the listview, a focus ring gets stuck on the breadcrumb root

Can any of the selection stories be crossed with drag and drop?

only odd case is still the highlight mode but only selection disabled, but I think that is what it is at this point, though should we swallow the spacebar on those elements? https://reactspectrum.blob.core.windows.net/reactspectrum/6db0e0700bb199c3577b53c1ba64fe1719512ee7/storybook/index.html?path=/story/listview-selection--disable-folder-selection&args=selectionStyle:highlight
it causes the listview to scroll

otherwise, this has my approval, tested a bit in chrome and it felt much more natural

adobe-bot · 2022-05-19T00:10:55Z

Build successful! 🎉

adobe-bot · 2022-05-19T00:34:24Z

Build successful! 🎉

ktabors · 2022-05-19T23:49:29Z

packages/@react-spectrum/progress/stories/ProgressBar.stories.tsx

+    (args) => render(args)
  )
  .add(
    'value: 50',


Would we eventually want this story to work with controls or remove it since controls allow us to adjust to 50 or 100(next story)?

Seems weird to have a hardcoded value of 50 in the story, but allow it to be edited in controls also?

I just edited my comment to suggest removing the hardcoded 50 and 100 stories without realizing you'd replied.

packages/@react-spectrum/progress/stories/ProgressBar.stories.tsx

ktabors · 2022-05-19T23:56:13Z

packages/@react-spectrum/progress/stories/ProgressCircle.stories.tsx

  .add(
    'Default',
-    () => render()
+    args => render(args)


This story used to have a value of 0, but because of the default args for the other stories it uses value=32. This default change is okay, but feels weird.

ktabors · 2022-05-19T23:59:19Z

packages/@react-spectrum/table/stories/Table.stories.tsx

+  .add(
+    'dynamic with disabled, multiple selection, highlight',
+    () => (
+      <TableView disabledKeys={['Foo 1', 'Foo 3']} aria-label="TableView with dynamic contents" selectionStyle="highlight" selectionMode="multiple" width={300} height={200} onSelectionChange={s => onSelectionChange([...s])}>


I wish there was a way to surface which keys are disabled without having to remember or check the code.

packages/@react-types/list/src/index.d.ts

ktabors · 2022-05-20T00:11:37Z

packages/@react-spectrum/list/stories/ListView.stories.tsx

-      <Item textValue="Adobe Illustrator">Adobe Illustrator</Item>
-      <Item textValue="Adobe XD">Adobe XD</Item>
+  .add('dynamic items + renderEmptyState', args => (<EmptyTest {...args} />))
+  .add('with ActionBar', args => <ActionBarExample {...args} />)


The controls don't work with this story because ActionBarExample wasn't updated to spread them. Is that something to fix now or with the CSF 3.0 refactor? I'm noticing this is true for some other stories too.

ktabors · 2022-05-20T00:16:14Z

packages/@react-spectrum/list/stories/ListView.stories.tsx

    )
+  )
  .add(
    'Drag within list (Reorder)',


I'm noticing the drag handler shows up after a drag. It is existing behavior.

I think because it becomes keyboard focused (intentionally I believe), so that causes the drag handle to also appear.

This is the only story I saw do this. 🤷

I'd expect drag between lists to behave the same, right? It doesn't show the drag handler after a drag from one list to the other.

ktabors · 2022-05-20T00:20:18Z

packages/@react-spectrum/list/stories/ListView.stories.tsx

-      <Item textValue="Adobe Photoshop">Adobe Photoshop</Item>
-      <Item textValue="Adobe Illustrator">Adobe Illustrator</Item>
-      <Item textValue="Adobe XD">Adobe XD</Item>
+  .add('overflowMode="truncate" (default)', args => (


Shouldn't this be a control and not its own story? Maybe the story is long text values?

Updated. Actually this uncovered a bug that the row height doesn't update when switching between overflow modes. Maybe we can address that separately though.

ktabors

Not a fan of controls persisting state for that story. I go to another story and come back and it remembers that state. I reloaded the browser and it remembered that state. There is the reset button... maybe I'll get used to it.

devongovett · 2022-05-20T00:34:35Z

Heh, I wanted the controls to persist between stories so if you set quiet then switch stories it would stay quiet. 🤷

adobe-bot · 2022-05-20T00:49:15Z

Build successful! 🎉

ktabors · 2022-05-20T16:21:46Z

Heh, I wanted the controls to persist between stories so if you set quiet then switch stories it would stay quiet. 🤷

I'm not seeing the persistence between stories, I'm only seeing it persisting within that story. In ListView stories, goto default, turn on quiet, goto dynamic items and it isn't quiet. Go back to default and it's still quiet.

ktabors

Thanks for the changes, looks good!

adobe-bot · 2022-05-20T17:09:41Z

Build successful! 🎉

adobe-bot · 2022-05-20T17:28:38Z

Build successful! 🎉

LFDanLu

LGTM, will do a small fix to one of the ListView dnd stories in a followup

devongovett and others added 8 commits May 11, 2022 16:39

Add storybook controls, reorganize ListView stories

6cb3af9

Enable interaction on non-selectable rows with onAction

6aba65f

Prevent selection of disabled items via keyboard with highlight selec…

5bccaba

…tion

Make actions primary on non-selectable rows

def50fb

Update selection style when prop changes

3739e99

Bug found via storybook controls

Drop addon-knobs

b60ffed

Revert useGridRow change for now

81cd136

fix lint

6dd5faf

devongovett added 2 commits May 12, 2022 13:11

Fix navigation in story example

08b040f

Merge branch 'listview-disabled-rows' of github.com:adobe/react-spect…

5373091

…rum into listview-disabled-rows

snowystinger mentioned this pull request May 13, 2022

Do not select disabled items #3126

Closed

5 tasks

Implement multiple disabled behaviors

c5821aa

devongovett added 4 commits May 17, 2022 13:03

Add example for disabling actions not selection

f19935b

drop disabledBehavior="action" and return states from useSelectableItem

6db0e07

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

f5e2d2a

…disabled-rows # Conflicts: # packages/@react-spectrum/list/stories/ListView.stories.tsx

snowystinger previously approved these changes May 18, 2022

View reviewed changes

Better prop description

ec6f80d

devongovett dismissed snowystinger’s stale review via ec6f80d May 19, 2022 00:04

Add aria-disabled and more tests

9038af1

devongovett marked this pull request as ready for review May 19, 2022 00:28

snowystinger previously approved these changes May 19, 2022

View reviewed changes

ktabors mentioned this pull request May 19, 2022

Update Storybook stories to CSF 3.0 format #3056

Closed

62 tasks

ktabors reviewed May 19, 2022

View reviewed changes

packages/@react-spectrum/progress/stories/ProgressBar.stories.tsx Show resolved Hide resolved

ktabors reviewed May 19, 2022

View reviewed changes

ktabors reviewed May 20, 2022

View reviewed changes

packages/@react-types/list/src/index.d.ts Outdated Show resolved Hide resolved

ktabors reviewed May 20, 2022

View reviewed changes

Review updates

fe1491c

devongovett dismissed snowystinger’s stale review via fe1491c May 20, 2022 00:43

snowystinger approved these changes May 20, 2022

View reviewed changes

ktabors approved these changes May 20, 2022

View reviewed changes

Merge branch 'main' into listview-disabled-rows

c919edf

Merge branch 'main' into listview-disabled-rows

fe7a971

LFDanLu approved these changes May 20, 2022

View reviewed changes

LFDanLu merged commit d750b23 into main May 20, 2022

LFDanLu deleted the listview-disabled-rows branch May 20, 2022 18:11

LFDanLu added needs testing ListView labels May 20, 2022

dannify removed the needs testing label Jun 3, 2022

Conversation

devongovett commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adobe-bot commented May 12, 2022

Uh oh!

adobe-bot commented May 12, 2022

Uh oh!

adobe-bot commented May 16, 2022

Uh oh!

adobe-bot commented May 18, 2022

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented May 19, 2022

Uh oh!

adobe-bot commented May 19, 2022

Uh oh!

ktabors May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ktabors May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktabors May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktabors left a comment

Choose a reason for hiding this comment

Uh oh!

devongovett commented May 20, 2022

Uh oh!

adobe-bot commented May 20, 2022

Uh oh!

ktabors commented May 20, 2022

Uh oh!

ktabors left a comment

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented May 20, 2022

Uh oh!

adobe-bot commented May 20, 2022

Uh oh!

LFDanLu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

devongovett commented May 11, 2022 •

edited

Loading

ktabors May 19, 2022 •

edited

Loading

ktabors May 20, 2022 •

edited

Loading

ktabors May 20, 2022 •

edited

Loading

LFDanLu left a comment •

edited

Loading