Skip to content

feat: Add embed-widget#1668

Merged
mofojed merged 5 commits intodeephaven:mainfrom
mofojed:embed-widget
Dec 7, 2023
Merged

feat: Add embed-widget#1668
mofojed merged 5 commits intodeephaven:mainfrom
mofojed:embed-widget

Conversation

@mofojed
Copy link
Copy Markdown
Member

@mofojed mofojed commented Dec 6, 2023

  • 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 Generic embed widget iframe #1629

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:

from deephaven.column import int_col, string_col
from deephaven.plot import Figure, express as dx
from deephaven import new_table
import matplotlib.pyplot as plt

t = new_table([
    string_col("Categories", ["A", "B", "C"]),
    int_col("Values", [1, 3, 5]),
])

t_rollup = t.rollup(aggs=[], by=["Categories"], include_constituents=True)

fig = Figure().plot_cat(series_name="Test", t=t, category="Categories", y="Values").show()

dx_fig = dx.bar(table=t, x="Categories", y="Values")

import matplotlib.pyplot as plt

mpl_fig = plt.figure()
ax = mpl_fig.subplots()
ax.plot([1, 2, 3, 4], [4, 2, 6, 7])

Then opened up pages to the following URLs to make sure the appeared correctly:

  1. http://localhost:4030/?name=t - Table displayed correctly
  2. http://localhost:4030/?name=t_rollup - Tree Table displayed correctly
  3. http://localhost:4030/?name=fig - Deephaven Figure displayed correctly
  4. http://localhost:4030/?name=dx_fig - Plotly-express figure displayed correctly
  5. http://localhost:4030/?name=mpl_fig - Error displayed correctly about unknown type
  6. http://localhost:4030/?name=err - Timeout error displayed
  7. http://localhost:4030/ - Error saying no name provided

- 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
@mofojed mofojed requested a review from mattrunyon December 6, 2023 15:38
@mofojed mofojed self-assigned this Dec 6, 2023
Copy link
Copy Markdown
Collaborator

@mattrunyon mattrunyon left a 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

Comment thread packages/embed-widget/src/App.tsx Outdated
Comment thread packages/embed-widget/src/App.tsx Outdated
Comment thread packages/embed-widget/src/App.tsx Outdated
Comment thread packages/embed-widget/src/App.tsx Outdated
Comment thread packages/embed-widget/src/App.tsx Outdated
Comment thread packages/embed-widget/src/App.tsx Outdated
/**
* 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`
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member Author

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.

Comment thread packages/embed-widget/src/App.tsx Outdated
Comment thread packages/embed-widget/src/index.tsx Outdated
Comment thread packages/embed-widget/src/logo.svg Outdated
Co-authored-by: Matthew Runyon <mattrunyonstuff@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 6, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (4c0200e) 46.64% compared to head (e80720b) 46.63%.
Report is 1 commits behind head on main.

Files Patch % Lines
packages/plugin/src/WidgetView.tsx 0.00% 9 Missing ⚠️
packages/components/src/ErrorBoundary.tsx 94.44% 1 Missing ⚠️
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              
Flag Coverage Δ
unit 46.63% <62.96%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Remove unused files
- Updated all create-react-app links to vite links where appropriate
- Updated embed-* documentation to point to the root readme
@mofojed mofojed requested a review from mattrunyon December 6, 2023 23:16
@mofojed mofojed merged commit 1b06675 into deephaven:main Dec 7, 2023
@mofojed mofojed deleted the embed-widget branch December 7, 2023 14:25
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generic embed widget iframe

2 participants