Skip to content
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 - ui plugins #408

Merged
merged 16 commits into from
May 9, 2024
Merged

Conversation

bmingles
Copy link
Contributor

@bmingles bmingles commented Apr 8, 2024

ui.list_view - basic items + table support

resolves #366

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Need to add some examples in the examples/README.md

I tried a couple of the examples as specified in the spec, and they didn't work as I was expecting. e.g. I can't change the selection here:

from deephaven import new_table
from deephaven.column import string_col, int_col

color_table = new_table([
    string_col("Keys", ["salmon", "lemonchiffon", "black"]),
    string_col("Labels", ["Salmon", "Lemon Chiffon", "Black"]),
    string_col("Descriptions", ["An interesting color", "Another interesting color", "A color"]),
    string_col("Icons", ["Amusementpark", "Teapot", "Sentiment Negative"]),
    string_col("SectionKeys", ["Interesting Colors", "Interesting Colors", "Other Colors"]),
    string_col("SectionLabels", ["Favorites", "Favorites", "Other"]),
    string_col("SectionDescriptions", ["Favorite colors", "Favorite colors", "Other colors"]),
    string_col("SectionIcons", ["Folder", "Folder", "Not Found"])
])

@ui.component
def ui_list_view():
    colors, set_colors = ui.use_state(["salmon", "lemonchiffon"])

    print(f"Colors are now {colors}")

    # this will create a controlled list_view with color_table
    return ui.list_view(
        color_table,
        key_column="Keys",
        label_column="Labels",
        description_column="Descriptions",
        icon_column="Icons",
        selected_keys=colors,
        on_change=set_colors
    )

my_list_view = ui_list_view()

@bmingles
Copy link
Contributor Author

@mofojed I think the issue with selection not working is you have to set selection_mode="multiple"

@bmingles
Copy link
Contributor Author

bmingles commented Apr 16, 2024

@mofojed I fixed the default selection_mode to be "MULTIPLE" and added handling for uppercase strings. Things should work as expected now

@bmingles bmingles force-pushed the 366-list-view-plugin branch 2 times, most recently from d667839 to e95f212 Compare April 18, 2024 20:22
bmingles added a commit to deephaven/web-client-ui that referenced this pull request Apr 19, 2024
- ListView components
- Smart item tooltip handling (only show when text is overflowing)
- Split out some shared util code from Picker
- Wrapped Text, View, and Heading in forwardRef

**Testing**
This can be tested using this PR:
deephaven/deephaven-plugins#408 . To make
TypeScript happy, you can install DHC as an alpha via:

```sh
npm run update-dh-packages -- --scope=@deephaven/js-plugin-ui -- 0.72.1-alpha-list-view.38
```

BREAKING CHANGE: `LIST_VIEW_ROW_HEIGHT` number constant replaced with
dictionary `LIST_VIEW_ROW_HEIGHTS`
@bmingles bmingles force-pushed the 366-list-view-plugin branch from e95f212 to 8bd5ae6 Compare April 19, 2024 15:20
@bmingles bmingles linked an issue Apr 22, 2024 that may be closed by this pull request
@bmingles bmingles force-pushed the 366-list-view-plugin branch 3 times, most recently from f685547 to be97314 Compare May 1, 2024 16:20
@bmingles bmingles marked this pull request as ready for review May 1, 2024 16:35
@bmingles bmingles requested a review from mofojed May 1, 2024 16:37
plugins/ui/docs/README.md Show resolved Hide resolved
plugins/ui/docs/README.md Outdated Show resolved Hide resolved
plugins/ui/docs/README.md Outdated Show resolved Hide resolved
Comment on lines 272 to 274
text = ui.text("Selection: " + ", ".join(map(str, value)), grid_column="span 2")

return ui.grid(text, lv, lv2, columns="repeat(2, 1fr)")
Copy link
Member

Choose a reason for hiding this comment

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

This example doesn't appear correctly for me as is. Currently the list views do not appear at all (the list-view-wrapper has a height of zero):
image

I'd just keep it simple and just return them in a column/default layout, no real need for a grid here after Don's changes, e.g.

Suggested change
text = ui.text("Selection: " + ", ".join(map(str, value)), grid_column="span 2")
return ui.grid(text, lv, lv2, columns="repeat(2, 1fr)")
text = ui.text("Selection: " + ", ".join(map(str, value)))
return text, lv, lv2

Or if we still want the list views side by side:

    text = ui.text("Selection: " + ", ".join(map(str, value)))

    return text, ui.flex(lv, lv2, flex_grow=1)

I'm guessing Don's flex/CSS changes broke this for you, but we'll need to update the example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same would happen to tables and plots with that ui.grid, it's not specific to list view. ui.grid needs a height specified for the rows, or for itself. We could consider better defaults on it. ui.grid would probably also benefit from a default gap etc.

Comment on lines +259 to +261
on_change=set_value,
selected_keys=value,
Copy link
Member

Choose a reason for hiding this comment

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

Selection isn't working for me at all right now. The second item should be selected initially, and it should update when things are checked off.

Looks like the props aren't actually passed through in @deephaven/components/ListView: https://github.com/deephaven/web-client-ui/blob/a283e13fafe1ecb156985fab00ba15344f180ff4/packages/components/src/spectrum/listView/ListView.tsx#L55

Unsure why that didn't get flagged as an unused variable ...

Also getting a ton of warnings in the logs whenever I change selection, we should squash these warnings or provide default values so Spectrum doesn't complain:

An aria-label or aria-labelledby prop is required for accessibility. 
    at $f85fb77f9d4cbc6c$var$ListView (

Copy link
Contributor Author

@bmingles bmingles May 3, 2024

Choose a reason for hiding this comment

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

Selection should be fixed by deephaven/web-client-ui#1986 .

I added the aria-label props to examples. I guess we could set a default of "List View" or something like that. @dsmmcken any opinions on this one?

@bmingles bmingles requested a review from mofojed May 3, 2024 16:23
@bmingles bmingles force-pushed the 366-list-view-plugin branch from b16f5cf to 28b5779 Compare May 8, 2024 18:45
@bmingles bmingles force-pushed the 366-list-view-plugin branch from 28b5779 to 5dd4551 Compare May 9, 2024 13:50
@bmingles
Copy link
Contributor Author

bmingles commented May 9, 2024

@mofojed Matt fixed the underlying e2e kerning issue, so this PR should be unblocked now

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Update the PR description before merging.

@bmingles bmingles merged commit ff7f769 into deephaven:main May 9, 2024
15 checks passed
@bmingles bmingles deleted the 366-list-view-plugin branch May 23, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui.list_view - basic items + table support
3 participants