-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ListView components #1919
Changes from 37 commits
ed6ed2a
2219115
5a98665
991042f
97d7412
b7f4df3
9fb7de1
2a04b31
f566e72
9b5a119
157e46f
85f87c3
9acfc30
4b2cd06
ebf1598
55d0fc2
9f4c1fb
c964da7
a111d5a
d488c92
dac492f
f2be02b
c7ecd14
663a965
195c42c
055f953
8d382b5
91deda8
962f64c
599902d
5fd74dd
5b7d5a2
4851669
36d4134
ae099ca
342b24c
a65e539
95a4744
383b98c
fb8e4a1
4feb7d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
import React, { useCallback, useState } from 'react'; | ||
import { Grid, Item, ListView, ItemKey, Text } from '@deephaven/components'; | ||
import { vsAccount, vsPerson } from '@deephaven/icons'; | ||
import { Icon } from '@adobe/react-spectrum'; | ||
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; | ||
import { generateNormalizedItems, sampleSectionIdAndClasses } from './utils'; | ||
|
||
// Generate enough items to require scrolling | ||
const itemsSimple = [...generateNormalizedItems(52)]; | ||
|
||
function AccountIcon({ | ||
slot, | ||
}: { | ||
slot?: 'illustration' | 'image'; | ||
}): JSX.Element { | ||
return ( | ||
// Images in ListView items require a slot of 'image' or 'illustration' to | ||
// be set in order to be positioned correctly: | ||
// https://github.com/adobe/react-spectrum/blob/784737effd44b9d5e2b1316e690da44555eafd7e/packages/%40react-spectrum/list/src/ListViewItem.tsx#L266-L267 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason they don't have a slot provider for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing @dsmmcken suggestion may be accurate here. Maybe they are trying to not overload the fact that icons can be included in actions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's a key difference between listview and listbox, https://react-spectrum.adobe.com/react-spectrum/ListBox.html#complex-items |
||
<Icon slot={slot}> | ||
<FontAwesomeIcon icon={vsAccount} /> | ||
</Icon> | ||
); | ||
} | ||
|
||
export function ListViews(): JSX.Element { | ||
const [selectedKeys, setSelectedKeys] = useState<'all' | Iterable<ItemKey>>( | ||
[] | ||
); | ||
|
||
const onChange = useCallback((keys: 'all' | Iterable<ItemKey>): void => { | ||
setSelectedKeys(keys); | ||
}, []); | ||
|
||
return ( | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
<div {...sampleSectionIdAndClasses('list-views')}> | ||
<h2 className="ui-title">List View</h2> | ||
|
||
<Grid columnGap={14} height="size-6000"> | ||
<Text>Single Child</Text> | ||
<ListView | ||
density="compact" | ||
gridRow="2" | ||
aria-label="Single Child" | ||
selectionMode="multiple" | ||
> | ||
<Item>Aaa</Item> | ||
</ListView> | ||
|
||
<label>Icons</label> | ||
<ListView | ||
gridRow="2" | ||
aria-label="Icon" | ||
density="compact" | ||
selectionMode="multiple" | ||
> | ||
<Item textValue="Item with icon A"> | ||
<AccountIcon slot="image" /> | ||
<Text>Item with icon A</Text> | ||
</Item> | ||
<Item textValue="Item with icon B"> | ||
<AccountIcon slot="image" /> | ||
<Text>Item with icon B</Text> | ||
</Item> | ||
<Item textValue="Item with icon C"> | ||
<AccountIcon slot="image" /> | ||
<Text>Item with icon C</Text> | ||
</Item> | ||
<Item textValue="Item with icon D"> | ||
<AccountIcon slot="image" /> | ||
<Text>Item with icon D with overflowing content</Text> | ||
</Item> | ||
</ListView> | ||
|
||
<label>Mixed Children Types</label> | ||
<ListView | ||
gridRow="2" | ||
aria-label="Mixed Children Types" | ||
density="compact" | ||
maxWidth="size-2400" | ||
selectionMode="multiple" | ||
defaultSelectedKeys={[999, 444]} | ||
> | ||
{/* eslint-disable react/jsx-curly-brace-presence */} | ||
{'String 1'} | ||
{'String 2'} | ||
{'String 3'} | ||
{''} | ||
{'Some really long text that should get truncated'} | ||
{/* eslint-enable react/jsx-curly-brace-presence */} | ||
{444} | ||
{999} | ||
{true} | ||
{false} | ||
<Item>Item Aaa</Item> | ||
<Item>Item Bbb</Item> | ||
<Item textValue="Complex Ccc"> | ||
<Icon slot="image"> | ||
<FontAwesomeIcon icon={vsPerson} /> | ||
</Icon> | ||
<Text>Complex Ccc with text that should be truncated</Text> | ||
</Item> | ||
</ListView> | ||
|
||
<label>Controlled</label> | ||
<ListView | ||
gridRow="2" | ||
aria-label="Controlled" | ||
selectionMode="multiple" | ||
selectedKeys={selectedKeys} | ||
onChange={onChange} | ||
> | ||
{itemsSimple} | ||
</ListView> | ||
</Grid> | ||
</div> | ||
); | ||
} | ||
|
||
export default ListViews; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should re-export this from
@deephaven/components
.Any reason why the default slot isn't
"illustration"
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just picked one of the 2 supported. Is
illustration
preferable overicon
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There don't appear to be any docs on this eh? I'm not sure what the difference is...
illustration
may actually be for an "illustrated message" whereasicon
is just something inline. So maybeicon
is better.And it seems there's an
Illustration
as well that putsillustration
in the slot, so the default slot should already beicon
when usingIcon
. Should we be re-exportingIllustration
?Doesn't seem to be an
Image
component, where'd theimage
slot come from?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the illustration slot gets be added on the icons coming from
@spectrum-icons/illustrations/xxxxx
https://react-spectrum.adobe.com/react-spectrum/ListView.html#complex-items
I think you are right that the default slot for
Icon
is'icon'
. Unfortunately the ListView has to haveimage
orillustration
vs Picker which supportsicon
. Maybe a Spectrum bug but not sure.I don't think there is an
Illustration
component to re-export unless we were to export the@spectrum-icons/illustrations/xxxxx
modules which seems to be an addon of some sortThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the expectation is you wouldn't put decorative icons in that slot, as it would be confusing with icons in the action slot (which I think in this example overflow because there is more than one)
Where as the primary slot is for images/illustrations, not icons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified things by just defaulting to "illustration" and removing the function arg. It's just for styleguide so probably doesn't matter too much anyway except to avoid confusing folks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the lack of exporting Illustration might be an oversight:
https://github.com/adobe/react-spectrum/blob/main/packages/%40adobe/react-spectrum/src/index.ts#L32
We could file a PR with Spectrum include
export {Illustration} from '@react-spectrum/icon';
, but as-is we don't have access to itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, they also don't export
UIIcon
. Unsure if intentional or not. Could open a PR and see how they respond.