feat: Picker - Table support for key + label columns#1876
feat: Picker - Table support for key + label columns#1876bmingles merged 40 commits intodeephaven:mainfrom
Conversation
b9dc6a5 to
0212185
Compare
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bd6cdef to
45509a7
Compare
| const keyColumn = getPickerKeyColumn(table, keyColumnName); | ||
|
|
||
| table | ||
| .seekRow(0, keyColumn, keyColumn.type, selectedKey) |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
79b8035 to
0df8f85
Compare
mofojed
left a comment
There was a problem hiding this comment.
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.
| getItemIndexByValue().then(index => { | ||
| if (index != null) { | ||
| setViewport(index); | ||
| if (index == null || isCanceled) { | ||
| return; | ||
| } | ||
|
|
||
| setViewport(index); | ||
| }); |
There was a problem hiding this comment.
Should we also be catching here? Otherwise we could have an uncaught promise. Probably just want to log the error anyway I guess.
There was a problem hiding this comment.
I went ahead and added an explicit catch / log
| import { getPickerKeyColumn } from './PickerUtils'; | ||
| import { usePickerItemRowDeserializer } from './usePickerItemRowDeserializer'; | ||
|
|
||
| const log = Log.module('Picker'); |
There was a problem hiding this comment.
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.
372442d to
4b70ed0
Compare
Initial table support for Picker
reuseItemsOnTableResizeflagTesting
Scroll to item fix for non-table data (this should work without any changes to plugins repo)
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
resolves #1858
resolves #1875