Skip to content

ListView docs#2734

Merged
devongovett merged 58 commits intomainfrom
ListView-docs
Jun 15, 2022
Merged

ListView docs#2734
devongovett merged 58 commits intomainfrom
ListView-docs

Conversation

@reidbarber
Copy link
Copy Markdown
Member

@reidbarber reidbarber commented Jan 7, 2022

Closes #2730

✅ 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:

Open ListView page of docs (react-spectrum/ListView.html). New link is in Collections section of sidenav.

🧢 Your Project:

RSP

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

Copy link
Copy Markdown
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review, will followup tmrw

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

Comment on lines +422 to +424
getDropOperation(target, types) {
let typesSet = types.types ? types.types : types;
let draggedTypes = [...typesSet.values()];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this has a typescript error since the type for types doesn't actually have types.type. There is a ticket for investigating why types returns a Set or a object with a types property that is a Set and either fixing that or updating the typescript type.

@adobe-bot
Copy link
Copy Markdown

Comment on lines +668 to +675
for (let item of e.items) {
for (let acceptedType of acceptedDragTypes) {
if (item.kind === 'text' && item.types.has(acceptedType)) {
let {id} = JSON.parse(await item.getText(acceptedType));
keysToMove.push(id);
}
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I would've liked to exit the inner loop early upon finding the matching acceptedType but e.items can either be an array of length X, where X is the number of dragged items OR is an array of a single item with a types key that is a Set of size X. The second case happens specifically if dragging multiple items that have different types via mouse.

Still digging into why this is and whether or not it is expected

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to be due to

for (let [type, items] of groupedByType) {
if (NATIVE_DRAG_TYPES.has(type)) {
// Only one item of a given type can be set on a data transfer.
// Join all of the items together separated by newlines.
let data = items.join('\n');
dataTransfer.items.add(data, type);
} else {
// Set data to the first item so we have access to the list of types.
dataTransfer.items.add(items[0], type);
}
}
if (needsCustomData) {
let data = JSON.stringify(customData);
dataTransfer.items.add(data, CUSTOM_DRAG_TYPE);
}

When there are multiple items w/ the same type or a item with multiple types present in the drag operation, we enter the custom data path, but if you drag a single folder + single item then we hit this path:

} else {
// Set data to the first item so we have access to the list of types.
dataTransfer.items.add(items[0], type);
}

resulting in a DataTransferItemList of two DataTransferItems

This will then hit the non-custom data path in readFromDataTransfer, eventually cumulating to

// All string items are different representations of the same item. There's no way to have
// multiple string items at once in the current DataTransfer API.
if (stringItems.size > 0) {
items.push({
kind: 'text',
types: new Set(stringItems.keys()),
getText: (type) => Promise.resolve(stringItems.get(type))
});

which will transform a stringItems list of

stringItems Map(2) {'file' => 'Adobe Photoshop', 'folder' => 'Documents'}

to a single item representation of
image

This feels like it is intentional due to limitations in the current DataTransferAPI, but the comment "All string items are different representations of the same item" sounds incorrect here? Not entirely sure what that means tbh, need to dig more into the DataTransferApi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all, it feels hard to tell between a drag + drop operation of a single file + folder and a drag + drop operation of a single item that has multiple types tied to it

@adobe-bot
Copy link
Copy Markdown

@snowystinger
Copy link
Copy Markdown
Member

I think one concern I have is, what is the difference between ListBox and ListView, how do I (a user) know which one I should be using?

<details>
<summary style={{fontWeight: 'bold'}}><ChevronRight size="S" /> Show code</summary>

```tsx example export=true render=false
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this example is a bit strange, you can empty the list, but once you do, you can't drag anything into it because root drop is not allowed. Until that point you can drag back and forth all you like.

Copy link
Copy Markdown
Member Author

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some super-nit suggestions (probably some are my typos tbh)

Comment on lines +304 to +314
By default, ListView uses the checkbox selection style, which includes a checkbox in each row for selection. When the `selectionStyle` prop is set to `"highlight"`, the checkboxes are hidden,
and the selected rows are displayed with a highlighted background instead.

In addition to changing the appearance, the selection behavior also changes depending on the `selectionStyle` prop. In the default checkbox selection style, clicking, tapping,
or pressing the <Keyboard>Space</Keyboard> or <Keyboard>Enter</Keyboard> keys toggles selection for the focused row. Using the arrow keys moves focus but does not change selection.

In the highlight selection style, however, clicking a row with the mouse _replaces_ the selection with only that row. Using the arrow keys moves both focus and selection.
To select multiple rows, modifier keys such as <Keyboard>Ctrl</Keyboard>, <Keyboard>Cmd</Keyboard>, and <Keyboard>Shift</Keyboard> can be used. To move focus without moving
selection, the <Keyboard>Ctrl</Keyboard> key on Windows or the <Keyboard>Option</Keyboard> key on macOS can be held while pressing the arrow keys. Holding this modifier while pressing
the <Keyboard>Space</Keyboard> key toggles selection for the focused row, which allows multiple selection of non-contiguous items. On touch screen devices, selection always behaves as toggle
since modifier keys may not be available. This behavior emulates native platforms such as macOS and Windows.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify with something like this:

By default, ListView uses the checkbox selection style, which includes a checkbox in each row for selection. When the selectionStyle prop is set to "highlight", the checkboxes are hidden, and the selected rows are displayed with a highlighted background instead.

In addition to changing the appearance, the selection behavior also changes depending on the selectionStyle prop. In the default checkbox selection style, clicking, tapping, or pressing the Space or Enter keys toggles selection for the focused row. Using the arrow keys moves focus but does not change selection.

In the highlight selection style, however, clicking a row with the mouse replaces the selection with only that row. Using the arrow keys moves both focus and selection. To select multiple rows, modifier keys such as Ctrl, Cmd, and Shift can be used. On touch screen devices, selection always behaves as toggle since modifier keys may not be available.

These selection styles implement the behaviors defined in Aria Practices.

Mostly the same except the last paragraph, which includes a bit fewer nitty gritty details that are defined in the Aria practices link.

On touch devices, `onAction` is always the primary tap interaction, and tapping on the row's checkbox or long pressing the row will select the row. In highlight selection, a long press will also shift the ListView into selection mode,
which displays checkboxes to perform selection. While the ListView is in this state, tapping the row will toggle selection instead of triggering `onAction`. Deselecting all items exits selection mode and hides the checkboxes.
Double clicking matches the behavior of desktop platforms like macOS and Windows, and a separate selection mode matches mobile platforms like iOS and Android.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would suggest to flip this around a bit to describe the different modes rather than by interaction method:

ListView supports row actions via the onAction prop, which is useful for functionality such as navigation. When nothing is selected, the ListView performs actions by default when clicking or tapping a row. Items may be selected using the checkbox, or by long pressing on touch devices. When at least one item is selected, the ListView is in selection mode, and clicking or tapping a row toggles the selection. Actions may also be triggered via the Enter key, and selection using the Space key.

This behavior is slightly different in the highlight selection style, where single clicking selects the row and actions are performed via double click. Touch and keyboard behaviors are unaffected.

please see the [drag and drop documentation](dragAndDrop.html).

The example below demonstrates how to create a draggable ListView and a droppable ListView.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dnd examples are good. We should probably do some API work separately so they can be simplified though. They are a bit overwhelming, and since many of these use cases are so common, we might be able to provide some helpers to make things easier.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll write a ticket

@devongovett
Copy link
Copy Markdown
Member

devongovett commented Jun 8, 2022

I think the anatomy diagram is a bit large, and I think only the first part showing the parts of the item are necessary rather than the whole listview. Also the font looks off... Gonna push an update for this, hope that's ok.

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

snowystinger
snowystinger previously approved these changes Jun 8, 2022
```tsx example
// Using the same list as above
<PokemonList disabledKeys={[3]} aria-label="ListView with disabled rows" />
```
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realized we don't cover disabledBehavior here. Probably a good followup item.

@adobe-bot
Copy link
Copy Markdown

@adobe-bot
Copy link
Copy Markdown

@devongovett devongovett merged commit 9b22962 into main Jun 15, 2022
@devongovett devongovett deleted the ListView-docs branch June 15, 2022 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ListView Docs

7 participants