-
Notifications
You must be signed in to change notification settings - Fork 16
feat: DH-18354 Add input filter support to UITable #1180
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
Conversation
ui docs preview (Available for 14 days) |
1 similar comment
ui docs preview (Available for 14 days) |
cbae21c
to
68f2ecf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements input filter support for UITable as part of DH-18354 and fixes issue #1080. Key changes include:
- Adding the dashboard core plugins to the Vite config.
- Updating transformNode and related tests to enforce unique element IDs (__dhId).
- Propagating a new widget "id" prop across WidgetHandler and its consumers, as well as passing down inputFilters to UITable.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
plugins/ui/src/js/vite.config.ts | Added alias for dashboard-core-plugins to support input filters. |
plugins/ui/src/js/src/widget/WidgetUtils.tsx | Updated transformNode to add __dhId propagation along with tests. |
plugins/ui/src/js/src/widget/WidgetHandler.tsx | Introduced and passed the "id" prop for widget identity tracking. |
plugins/ui/src/js/src/widget/WidgetHandler.test.tsx | Updated tests to account for new id prop requirements. |
plugins/ui/src/js/src/widget/DashboardWidgetHandler.tsx | Propagated the id prop to the WidgetHandler. |
plugins/ui/src/js/src/layout/ReactPanel.tsx | Wrapped content with DhIdContext to support the new id flow. |
plugins/ui/src/js/src/elements/utils/ElementUtils.tsx & tests | Added __dhId propagation to ObjectView wrapping of exported objects. |
plugins/ui/src/js/src/elements/UITable/UITable.tsx | Imported and passed inputFilters to IrisGrid from dashboard core plugins. |
plugins/ui/src/js/src/elements/ObjectView.tsx | Updated ObjectView props to accept and propagate __dhId. |
plugins/ui/src/js/package.json | Updated dependency versions to integrate the input filter changes. |
Comments suppressed due to low confidence (2)
plugins/ui/src/js/src/widget/WidgetHandler.tsx:215
- [nitpick] Consider renaming the 'id' parameter passed to transformNode (and the corresponding prop across components) to a more descriptive name like 'rootId' to clarify its purpose in the unique __dhId generation.
const hydratedDocument = transformNode(doc, (key, value) => {
plugins/ui/src/js/package.json:43
- Double-check that the dependency version changes (e.g. downgrading from 0.109.0 to 0.108.1-input-filters.21) are intentional and compatible with the rest of the ecosystem.
"@deephaven/chart": "0.108.1-input-filters.21",
ui docs preview (Available for 14 days) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing some strange IDs when using tabs.
@@ -527,6 +528,8 @@ export function UITable({ | |||
}; | |||
}, [irisGridServerProps, initialHydratedState]); | |||
|
|||
const inputFilters = useDashboardColumnFilters(model?.columns ?? EMPTY_ARRAY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be passing the table here as well if the model has a table
.
What else do we need for ChartBuilder? I guess we need a stable way to fetch the table as well so we can refetch it on reload...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table for chart builder is actually sent with the event (not a fetch function). Although we could send the fetch function, but that would change if the table changes. So not sure how we would want to handle that.
Other than that, there's a relatively short handleCreateChart
function in IrisGridPanel
that would need to have its logic added to a hook that can be used by GridWidgetPlugin
and UITable
and pass the handler to IrisGrid
in each of those
ui docs preview (Available for 14 days) |
ui docs preview (Available for 14 days) |
I need to add |
ui docs preview (Available for 14 days) |
ui docs preview (Available for 14 days) |
ui docs preview (Available for 14 days) |
ui docs preview (Available for 14 days) |
This builds on top of #1180 and the actual linker code is relatively simple because of the hook from deephaven/web-client-ui#2459 Tested with test plan in DH-18836
Fixes #1080
Until deephaven/web-client-ui#2438 is merged, released, and updated in core, e2e tests will timeout after a few hrs, so I just cancelled the e2e run
I tested with this snippet. Then added an input filter and checked that all columns showed up and worked. Also tested clear all filters button and shortcut