feat: DH-21344: Dropdown for input table enums#2704
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2704 +/- ##
==========================================
+ Coverage 50.93% 51.11% +0.17%
==========================================
Files 794 798 +4
Lines 45211 45677 +466
Branches 11510 11652 +142
==========================================
+ Hits 23028 23346 +318
- Misses 22164 22312 +148
Partials 19 19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces column-restriction–driven cell editors for input tables by adding a registry-based mechanism to @deephaven/grid, wiring that registry through @deephaven/iris-grid via context, and providing a default dropdown editor for string-list (enum-like) restrictions.
Changes:
- Add
ColumnRestrictionsupport and acellInputRendererRegistryto@deephaven/gridto render custom cell editors based on restriction type. - Add
IrisGridContextProvider+ default registry plumbing in@deephaven/iris-grid, including a dropdown editor (CellDropdownField) for string-list restrictions. - Add an IrisGrid key handler to preserve existing values for restriction-backed editors when editing is initiated by typing.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/iris-grid/src/key-handlers/RestrictedEditKeyHandler.ts | Intercepts printable-key edits to open restriction-backed editors without overwriting existing values. |
| packages/iris-grid/src/key-handlers/index.ts | Exports the new key handler. |
| packages/iris-grid/src/IrisGridModel.ts | Adds getColumnRestriction and defines string-list restriction type helpers. |
| packages/iris-grid/src/IrisGridContextProvider.tsx | Combines theme + renderer registry into a single context IrisGrid can consume. |
| packages/iris-grid/src/IrisGrid.tsx | Switches IrisGrid to the combined context and passes the registry down to Grid. |
| packages/iris-grid/src/IrisGrid.scss | Adds styling so dropdown editors get a visible focus/outline treatment. |
| packages/iris-grid/src/index.ts | Exposes new context/provider and dropdown/editor context exports. |
| packages/iris-grid/src/CellInputRendererContext.tsx | Defines default renderer registry and maps string-list restrictions to the dropdown editor. |
| packages/iris-grid/src/CellDropdownField.tsx | Implements the dropdown cell editor using Spectrum Picker wrapper. |
| packages/iris-grid/src/CellDropdownField.scss | Styles dropdown editor to fit within grid cell geometry and visuals. |
| packages/grid/src/key-handlers/EditKeyHandler.ts | Adds Enter-to-start-editing behavior for editable cells. |
| packages/grid/src/index.ts | Re-exports CellInputField, renderer types, and GridModel types. |
| packages/grid/src/GridRendererTypes.ts | Adds CellInputRendererFn and CellInputRendererRegistry types. |
| packages/grid/src/GridModel.ts | Adds ColumnRestriction type and getColumnRestriction hook on models. |
| packages/grid/src/Grid.tsx | Uses cellInputRendererRegistry to render custom editors and sets CSS vars for editor styling. |
| packages/app-utils/src/components/AppBootstrap.tsx | Wraps app tree with IrisGridContextProvider to supply the combined context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| // Compute the stripe color for this visible row, mirroring how the canvas renderer draws row stripes | ||
| let cellBackgroundColor: string | undefined; | ||
| if (rowBackgroundColors) { | ||
| const colorSets = GridRenderer.getCachedBackgroundColors( | ||
| rowBackgroundColors, | ||
| maxDepth | ||
| ); | ||
| const depth = isExpandableGridModel(model) ? model.depthForRow(row) : 0; | ||
| const colorSet = colorSets[row % colorSets.length]; | ||
| cellBackgroundColor = colorSet[Math.min(depth, colorSet.length - 1)]; | ||
| } |
There was a problem hiding this comment.
Hmm this doesn't take into account if the cell background has been set via formatting. I don't particularly like how it's duplicating a bunch of the logic in GridRenderer either, but I don't have a good answer for that.
There was a problem hiding this comment.
Formatting is now taken into account. I tried to refactor for common code, but the GridRenderer does row stripes and formats and two different passes, so I ended up with something that seemed less readable (which I did not commit).
| const renderer = | ||
| columnRestrictions.length === 1 | ||
| ? cellInputRendererRegistry?.get(columnRestrictions[0].type) | ||
| : undefined; |
There was a problem hiding this comment.
So if column restrictions has more than one element, we just use undefined? Then what's the point of having an array for column restrictions?
There was a problem hiding this comment.
We need to discuss this. It is an array of restrictions because a user can add as many as they want. I should have a comment on this code, but I think this code is essentially saying, "The UI doesn't know how to render multiple column restrictions, so if you have more than one then we use the default cell renderer."
That being said, maybe there is a discussion to be had about how we render columns with multiple restrictions. But I feel like it would be difficult to create an algorithm or hierarchy that would handle all cases correctly given that a user can combine an restrictions even if they make no sense.
| * value rather than replacing it with the typed character. | ||
| */ | ||
| export type CellInputRendererFn = (( | ||
| props: CellInputFieldProps & { columnRestrictions: ColumnRestriction[] } |
There was a problem hiding this comment.
I think just restrictions makes more sense here? Is there a reason to specify the restrictions are for the whole column?
From the perspective of the cell input, it should only matter what the restrictions are for that cell. Doesn't matter if those restrictions are applied to the whole column or just that row for some reason.
Also, should be a readonly array provided.
Also strange how it's tacked on to the type here, instead of this type being defined on it's own. Feel like it should just be on CellInputProps and that's defined in this file, and then CellInputField should reference this for it's props (just ignoring the restrictions in that case).
Also weird that we have an onContextMenu prop that is never used. I wonder why that is. Maybe we should just remove that prop.
| cursorColumn == null || | ||
| cursorRow == null || | ||
| !isEditableGridModel(model) || | ||
| !model.isEditableRange(GridRange.makeCell(cursorColumn, cursorRow)) |
There was a problem hiding this comment.
Hmm I see this is already in the EditKeyHandler, but it's incorrect that we're passing the visible column/row down instead of the model index. I think this might cause problems if you move a key column and then try to edit.
| * Key handler that intercepts printable-character keystrokes on cells whose | ||
| * column restriction type has a registered custom renderer (e.g. a dropdown). | ||
| * |
There was a problem hiding this comment.
Why isn't this just baked into the EditKeyHandler? I don't think any of this is IrisGrid specific.
| import { SELECTION_DIRECTION } from '@deephaven/grid'; | ||
| import './CellDropdownField.scss'; | ||
|
|
||
| export type CellDropdownFieldProps = { |
There was a problem hiding this comment.
Should extend the CellInputProps as mentioned in my other comment.
|
|
||
| class IrisGrid extends Component<IrisGridProps, IrisGridState> { | ||
| static contextType = IrisGridThemeContext; | ||
| static contextType = IrisGridContext; |
There was a problem hiding this comment.
This is a breaking change now; existing users of IrisGridThemeContext (including ThemeBootstrap) will not be wired up correctly; they need to add the IrisGridContextProvider as a child to the ThemeBootstrap (since IrisGridContextProvider still uses the IrisGridThemeContext). Though having IrisGridContextProvider makes more sense when it's providing more things than just the theme.
Hmm. Obviously functional component would make this a little bit easier to handle both contexts, but that's a huge change...
I think instead, I'd just rename IrisGridThemeProvider and IrisGridThemeContext to IrisGridContextProvider and IrisGridContext, have all the functionality in there, and re-export as IrisGridThemeProvider/IrisGridThemeContext with a deprecated tag. Then we don't break existing users/UIs, and we can build on top of it.
| return null; | ||
| } | ||
|
|
||
| getColumnRestriction(column: ModelIndex): ColumnRestriction[] { |
There was a problem hiding this comment.
getColumnRestriction - but it returns an array? Should it be getColumnRestrictions, or are we just sticking to one restriction for each column? We only seem to use one restriction in the renderer anyways, so why do we allow an array?
There was a problem hiding this comment.
Updated to be plural, but per my earlier comment, we should discuss what plural restrictions means.
|
How should I go about testing/using this? |
I've been testing by running the branch with latest |
Co-authored-by: Mike Bender <mikebender@deephaven.io>
Consumes the new Column Restriction JS API to add an dropdown cell renderer for input tables. The renderer is pass to Grid via a function in a registry in a context around IrisGrid. This will allow Enterprise to register more input table cell renderers.