-
Notifications
You must be signed in to change notification settings - Fork 31
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: Add embed-widget #1668
feat: Add embed-widget #1668
Conversation
- Loads the widget - Fixes deephaven#1629
- Loads the widget and displays it if there is a widget plugin that matches - Displays an error if no plugin is found that can display the type of widget, or a widget with that name is not found - Fixes deephaven#1629
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.
Are we planning to keep embed-grid
and embed-chart
. If not, I'd probably remove in a separate PR. They are redundant although each would have a smaller bundle for initial load
/** | ||
* A functional React component that displays a Deephaven Widget using the @deephaven/plugin package. | ||
* It will attempt to open and display the widget specified with the `name` parameter, expecting it to be present on the server. | ||
* E.g. http://localhost:4030/?name=myWidget will attempt to open a widget `myWidget` |
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.
I'm not sure if it would make much difference, but we could add an optional type
query param and fallback to the type lookup if it's omitted. Would let us be explicit in Jupyter for a more efficient lookup. Not sure if fetching the exported variable list could be expensive in any situations
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.
Yea it probably would never be too expensive, but we certainly can add type
as an optional param.
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.
Actually, we can't really save anything here - when we call getObject with just the name/type defined, it still looks up the variable definition object: https://github.com/deephaven/deephaven-core/blob/374188ae166aa202da30e078a78c13334cb683f6/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java#L803
If we pass it the instance of the variable definition, we skip up looking up that variable definition: https://github.com/deephaven/deephaven-core/blob/374188ae166aa202da30e078a78c13334cb683f6/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java#L789
So providing type
can basically just get you in a pickle, if it doesn't match what's on the server, and it's not saving us anything since we need to get the definition anyway.
Co-authored-by: Matthew Runyon <mattrunyonstuff@gmail.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1668 +/- ##
==========================================
- Coverage 46.64% 46.63% -0.01%
==========================================
Files 599 604 +5
Lines 36681 36753 +72
Branches 9196 9211 +15
==========================================
+ Hits 17109 17139 +30
- Misses 19520 19562 +42
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Remove unused files - Updated all create-react-app links to vite links where appropriate - Updated embed-* documentation to point to the root readme
Tested by installing the matplotlib, plotly-express python plugins, but only installing the plotly-express JS plugin (not the matplotlib JS plugin). Then ran the following snippet:
Then opened up pages to the following URLs to make sure the appeared correctly: