feat: ListView components #1919
Conversation
bmingles
force-pushed
the
1909-list-view-components
branch
2 times, most recently
from
281cf5d to
b6d1626
Compare
bmingles
force-pushed
the
1909-list-view-components
branch
from
8e4f053 to
a65e539
Compare
| 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'; |
There was a problem hiding this comment.
Should re-export this from @deephaven/components.
Any reason why the default slot isn't "illustration"?
There was a problem hiding this comment.
I just picked one of the 2 supported. Is illustration preferable over icon ?
There was a problem hiding this comment.
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" whereas icon is just something inline. So maybe icon is better.
And it seems there's an Illustration as well that puts illustration in the slot, so the default slot should already be icon when using Icon. Should we be re-exporting Illustration?
Doesn't seem to be an Image component, where'd the image slot come from?
There was a problem hiding this comment.
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 have image or illustration vs Picker which supports icon. 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 sort
There was a problem hiding this comment.
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.
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.
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 it
There was a problem hiding this comment.
Perhaps, they also don't export UIIcon. Unsure if intentional or not. Could open a PR and see how they respond.
| @@ -22,6 +22,7 @@ | |||
| "build:sass": "sass --embed-sources --load-path=../../node_modules ./src:./dist" | |||
| }, | |||
| "dependencies": { | |||
| "@adobe/react-spectrum": "^3.34.1", | |||
There was a problem hiding this comment.
Should import everything from @deephaven/components, even in jsapi-components. Seems we're importing useProvider in ListView.tsx.
There was a problem hiding this comment.
@mofojed hmm, we aren't re-exporting currently. Any opinions on how we want to do this? useProvider seems too generic, but useSpectrumProvider seems a bit odd since we are trying to abstract away Spectrum.
There was a problem hiding this comment.
Well, you're exporting SpectrumThemeProvider, useSpectrumThemeProvider to mirror that would be fine with me.
Unsure if we want to get more abstract than that, we already have ThemeProvider.
| 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.
Any reason they don't have a slot provider for icon? We could even add a PR for it... seems odd they support image and illustration but not icon.
There was a problem hiding this comment.
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.
Yeah, that's a key difference between listview and listbox, https://react-spectrum.adobe.com/react-spectrum/ListBox.html#complex-items
Testing
This can be tested using this PR: deephaven/deephaven-plugins#408 . To make TypeScript happy, you can install DHC as an alpha via:
BREAKING CHANGE:
LIST_VIEW_ROW_HEIGHTnumber constant replaced with dictionaryLIST_VIEW_ROW_HEIGHTS