feat: Add ui.resolve to support URIs in dh.ui #1215
Conversation
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
Minor doc change, a couple comments, otherwise looks great!
A couple things (which I think could be done in a follow up ticket) that would be nice:
1.Doing a loading state for ComboBox, ListView, Picker while the table source is loading
2. Allow passing a URI directly into item_table_source and/or ui.table and automagically handle it as a ui.resolve
| */ | ||
| [ELEMENT_KEY]: K; | ||
| props?: P; | ||
| props: P; |
There was a problem hiding this comment.
Do we actually enforce this? I thought we might omit the props key entirely if there were no props on the element, but I could be mistaken.
There was a problem hiding this comment.
So NodeRenderer would omit the props if they don't exist. However, because of things like BaseElement we essentially always have props even if they're empty.
I can't recall exactly why I made this required, so I'll double check it's still needed
There was a problem hiding this comment.
Ahh I added this because my UriNode which specifies the props is still listed w/ possibly undefined props because of this. The generic lets you specify the props, but then still says the props may be undefined
There was a problem hiding this comment.
Nice, I think you've got it covered now.
There was a problem hiding this comment.
I made this a little nice and used a conditional type to check if it is possibly undefined to make props optional if it is. Removes the need for props: {} in tests that don't care about it
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
plugins/ui/docs/components/uri.md
Outdated
| ### Plain references | ||
|
|
||
| One way to use `ui.resolve` is to assign the reference to a variable which will be opened by the web UI just like you created the resource in the first place. This can be useful if you want to display tables from multiple sources in a single dashboard. |
There was a problem hiding this comment.
Should make it clear that you want to display widgets/tables from multiple sources in a single dashboard without incurring the cost.
| One way to use `ui.resolve` is to assign the reference to a variable which will be opened by the web UI just like you created the resource in the first place. This can be useful if you want to display tables from multiple sources in a single dashboard. | |
| One way to use `ui.resolve` is to assign the reference to a variable which will be opened by the web UI just like you created the resource in the first place. This can be useful if you want to display tables from multiple sources in a single dashboard, without the worker defining the dashboard having to pull those resources in. |
Maybe we should just have a diagram in Mermaid, showing how it goes. Something like it going from Worker B -> Worker A -> Client to just Worker B -> Client (but make it a pretty diagram).
plugins/ui/docs/components/uri.md
Outdated
| Deephaven UI provides a `resolve` method (not to be confused with `resolve` method from the Deephaven URI package) that allows you to reference Deephaven resources from other instances. Unlike the Deephaven URI package, `ui.resolve` does not resolve the URI to its resource on the server, so you cannot apply operations to the resource. | ||
|
|
||
| > [!NOTE] | ||
| > The only valid URIs for deephaven UI at the moment are for Deephaven Enterprise persistent queries. |
There was a problem hiding this comment.
We should probably have a follow up ticket to support the Core URI scheme as well: https://deephaven.io/core/docs/reference/data-import-export/uri/
|
|
||
| ## URI Encoding | ||
|
|
||
| If your URI contains any special characters, such as spaces or slashes, you must encode the URI components using standard URL encoding. This is because URIs are often used in web contexts where special characters can cause issues. You can use Python's built-in `urllib.parse.quote` function to encode your URIs. |
There was a problem hiding this comment.
Mixed feelings here... should we be explaining this on the Core side? Or on the Enterprise side? We really should have a utility method that accepts the query name and object name to make the URI for you, with proper encoding, ui.query_resolve or something, e.g. ui.query_resolve(query="MyQueryName", widget="MyWidget"). Technically it'd be Core+ only, but I wouldn't feel terrible about putting it in Core anyways...
As for this part - "If your URI contains any special characters" - moreso should be "You can construct a URI with any query name. If your query name contains special characters, such as spaces or slashes, you must encode the name first to avoid mangling the URL". (But nicer)
| if (isLoading || table == null || api == null) { | ||
| return ( | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| <DHComboBox loadingState="loading" {...pickerProps}> |
There was a problem hiding this comment.
When does this state actually occur? And I was thinking we'd want something similar for the error state too, rather than the WidgetErrorView which completely shift the layout. Stretch goal though.
There was a problem hiding this comment.
This is somewhat visible when loading from a PQ sometimes (especially first fetch)
It's possibly more visible when loading via name because the check has been modified to ensure the serial doesn't exist in QueryConfig table. Previously it was not searching for both, but I think that's a race condition when loading directly to a dashboard as the JS API might not have received all of the query added events yet
| */ | ||
| [ELEMENT_KEY]: K; | ||
| props?: P; | ||
| props: P; |
There was a problem hiding this comment.
Nice, I think you've got it covered now.
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
There was a problem hiding this comment.
Thoughts on whether we should pull this into Grizzly as well? I think would just need to cherry-pick back your change? deephaven/web-client-ui#2509
It's slated for G+, so not going to risk it, but just means we'll need users to update to G+ for the latest deephaven.ui.
There was a problem hiding this comment.
Mmm I could see if it picks back cleanly. It's mostly types on the web-client-ui side with the only change really being in useWidget (and alias useTableClose as useWidgetClose). And I think the DHE side would be the same for grizzly as G+ here
I guess it depends what our thoughts are on how hard we want to push people to G+ soon. This would be a very compelling reason to upgrade if the rest of the upgrade is easy
3 participants
Tested with DH-19001 test plan on
mattrunyon-gplusStill needs