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: Picker - Table support for key + label columns #1876

Merged
merged 40 commits into from
Mar 27, 2024

Conversation

bmingles
Copy link
Contributor

@bmingles bmingles commented Mar 18, 2024

Initial table support for Picker

  • key and value column support
  • Scroll to selected item when popover opens
  • Viewport data now supports re-use of existing item objects via reuseItemsOnTableResize flag

Testing

Scroll to item fix for non-table data (this should work without any changes to plugins repo)

import deephaven.ui as ui
from deephaven.ui import use_state

items = list("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz")

@ui.component
def picker():
    value, set_value = use_state("M")

    # Picker for selecting values
    pick = ui.picker(
        label="Text",
        on_selection_change=set_value,
        selected_key=value,
        children=items
    )

    # Show current selection in a ui.text component
    text = ui.text("Selection: " + value)

    # Display picker and output in a flex column
    return ui.flex(
        pick,
        text,
        direction="column",
        margin=10,
        gap=10,
    )


p = picker()

Table support

There is a draft PR in the plugins repo that enables the table support on the plugins side of things (note that it currently will have type errors but should run)
deephaven/deephaven-plugins#382

Here's an example that demonstrates a large number of items that also tick

import deephaven.ui as ui
from deephaven.ui import use_state
from deephaven import time_table
import datetime

mylist = ["mars", "pluto", "saturn"]

simple_ticking = time_table("PT2S", start_time=datetime.datetime.now() - datetime.timedelta(seconds=2000)).update([
    'Id=(new Integer(i * 100))',
    "Planet=mylist[ (int) (3 * Math.random()) % 3 ]",
])


@ui.component
def picker():
    value, set_value = use_state(13800)

    print("Test", value)

    # Picker for selecting values
    pick = ui.picker(
        simple_ticking,
        key_column="Id",
        label_column="Planet",
        label="Text",
        on_selection_change=set_value,
        selected_key=value,
    )

    # Show current selection in a ui.text component
    text = ui.text("Selection: " + str(value))

    # Display picker and output in a flex column
    return ui.flex(
        pick,
        text,
        direction="column",
        margin=10,
        gap=10,
    )


picker_table = picker()

resolves #1858
resolves #1875

@bmingles bmingles force-pushed the 1858-picker-table-support branch 2 times, most recently from b9dc6a5 to 0212185 Compare March 20, 2024 17:47
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 68.13187% with 58 lines in your changes are missing coverage. Please review.

Project coverage is 46.20%. Comparing base (78c1af1) to head (4b70ed0).

Files Patch % Lines
...es/jsapi-components/src/spectrum/Picker/Picker.tsx 0.00% 29 Missing ⚠️
packages/components/src/spectrum/picker/Picker.tsx 0.00% 24 Missing ⚠️
packages/jsapi-utils/src/TableUtils.ts 85.71% 3 Missing ⚠️
packages/code-studio/src/styleguide/Pickers.tsx 80.00% 1 Missing ⚠️
packages/utils/src/TestUtils.ts 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1876      +/-   ##
==========================================
+ Coverage   46.11%   46.20%   +0.09%     
==========================================
  Files         637      641       +4     
  Lines       38054    38216     +162     
  Branches     9620     9671      +51     
==========================================
+ Hits        17549    17659     +110     
- Misses      20452    20504      +52     
  Partials       53       53              
Flag Coverage Δ
unit 46.20% <68.13%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bmingles bmingles changed the title feat: Use KeyedItem interface in Picker feat: Picker - Table support for key + label columns Mar 22, 2024
@bmingles bmingles marked this pull request as ready for review March 22, 2024 17:11
@bmingles bmingles requested a review from mofojed March 22, 2024 17:39
const keyColumn = getPickerKeyColumn(table, keyColumnName);

table
.seekRow(0, keyColumn, keyColumn.type, selectedKey)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like passing keyColumn.type directly here won't always work. I'm getting a java.lang.UnsupportedOperationException: Invalid value type for seekRow: java.lang.String error in certain scenarios. Will have to see about mapping this to expected type.


const index = await table.seekRow(0, column, columnValueType, value);

return index === -1 ? null : index;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding correctly, the -1 should be returned for non-matches once deephaven/deephaven-core/pull/5238 is merged. For now, the non-match scenario will return an actual row index. In cases where a table ticks causing a previous selection to drop from the table, this will cause scrolling to a seemingly arbitrary position, but this should be enough of an edge case to not be too worried about it since it is temporary

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.

Great stuff! Some little cleanup.
As you mentioned in the ticket, it does not handle description_column or icon_column - I feel like those should be pretty easy to wire up, just need to include it in normalized items. I added a comment on #1890 to include icon_column as well. Using a snippet like:

import deephaven.ui as ui
from deephaven.ui import use_state
from deephaven import time_table
import datetime

mylist = ["mars", "pluto", "saturn"]
colors = ["salmon", "lemonchiffon", "purple", "green", "red"]
icons = ["vsEdit", "vsTrash"]

simple_ticking = time_table("PT2S", start_time=datetime.datetime.now() - datetime.timedelta(seconds=2000)).update([
    'Id=(new Integer(i * 100))',
    "Planet=(String)mylist[ (int) (3 * Math.random()) % 3 ]",
    "Color=(String)colors[ (int) (5 * Math.random()) % 5 ]",
    "Icon=(String)icons[ (int) (2 * Math.random()) % 2 ]"
])


@ui.component
def picker():
    value, set_value = use_state(13800)

    print("Test", value)

    # Picker for selecting values
    pick = ui.picker(
        simple_ticking,
        key_column="Id",
        label_column="Planet",
        description_column="fdsafdsa",
        icon_column="Icon",
        label="Text",
        on_selection_change=set_value,
        selected_key=value,
    )

    # Show current selection in a ui.text component
    text = ui.text("Selection: " + str(value))

    # Display picker and output in a flex column
    return ui.flex(
        pick,
        text,
        direction="column",
        margin=10,
        gap=10,
    )


picker_table = picker()

That can be after this merges though.

packages/components/src/spectrum/picker/Picker.tsx Outdated Show resolved Hide resolved
packages/components/src/spectrum/picker/Picker.tsx Outdated Show resolved Hide resolved
packages/components/src/spectrum/picker/Picker.tsx Outdated Show resolved Hide resolved
packages/jsapi-components/src/spectrum/Picker/Picker.tsx Outdated Show resolved Hide resolved
mofojed
mofojed previously approved these changes Mar 27, 2024
Comment on lines 84 to 90
getItemIndexByValue().then(index => {
if (index != null) {
setViewport(index);
if (index == null || isCanceled) {
return;
}

setViewport(index);
});
Copy link
Member

Choose a reason for hiding this comment

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

Should we also be catching here? Otherwise we could have an uncaught promise. Probably just want to log the error anyway I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and added an explicit catch / log

mofojed
mofojed previously approved these changes Mar 27, 2024
import { PICKER_ITEM_HEIGHT, PICKER_TOP_OFFSET } from '@deephaven/utils';
import { useCallback, useEffect, useMemo } from 'react';
import useGetItemIndexByValue from '../../useGetItemIndexByValue';
import { useViewportData } from '../../useViewportData';
import { getPickerKeyColumn } from './PickerUtils';
import { usePickerItemRowDeserializer } from './usePickerItemRowDeserializer';

const log = Log.module('Picker');
Copy link
Member

Choose a reason for hiding this comment

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

One problem with log API is it won't disambiguate between any other 'Picker' components. Could ensure it's "unique" by prefixing it with jsapi-components... doesn't matter much to me.

@bmingles bmingles merged commit bfbf7b1 into deephaven:main Mar 27, 2024
5 checks passed
@bmingles bmingles deleted the 1858-picker-table-support branch March 27, 2024 18:07
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui.picker - Scroll to selected item Picker - table support
2 participants