-
Notifications
You must be signed in to change notification settings - Fork 1.2k
PerspectiveWorkspace React component #3044
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
base: master
Are you sure you want to change the base?
PerspectiveWorkspace React component #3044
Conversation
90b0462 to
b60ab32
Compare
| widget: pspWorkspace.PerspectiveViewerWidget; | ||
| isGlobalFilter: boolean; | ||
| }) => void; | ||
| id?: string; |
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.
Use React.HTMLAttributes
| PerspectiveWorkspace, | ||
| } from "@finos/perspective-react"; | ||
|
|
||
| import "@finos/perspective-viewer/dist/css/themes.css"; |
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.
Don't import themes.css for viewers within a workspace - use the specific theme pro.css (in this case)
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.
This is for the <PerspectiveViewer /> tests, <PerspectiveWorkspace /> tests used themes.css as well though so I removed that line.
| await new Promise((r) => setTimeout(r, delay)); | ||
| } | ||
| return false; | ||
| } |
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.
If every one of Perspective's tests did this, the test suite would run ~2hrs. You have multiple options available:
- Listen for
perspective-config-update - Remove the button click from the tests and just await the workspace calls directly (through an imperative handle for testing only).
- Await
workspace.flush()
| master: { sizes: [] }, | ||
| detail: { sizes: [] }, | ||
| master: undefined, | ||
| detail: { main: null }, |
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.
This does not look right
| }; | ||
| } | ||
|
|
||
| export function genId(workspace: PerspectiveWorkspaceConfig) { |
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.
Please remove this - IDs are an internal implementation detail, there is no reason to expose this in the public API.
| const exists = document.querySelector("body > perspective-indicator"); | ||
| if (exists) { | ||
| return exists as HTMLElement; | ||
| } |
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.
This smells like a previous <perspective-workspace> that was disconnected leaked this element, not the responsibility of this method to correct.
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.
Sure, I set the constructed indicator to be a child of <perspective-workspace> so that the element is cleaned up when the workspace is unmounted.
| } | ||
|
|
||
| export interface PerspectiveWorkspaceConfig<T> { | ||
| // TODO: I have made some changes to this interface because it seemed to not reflect reality. |
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.
Lumino's DockPanel.saveLayout() does not return string identifiers for widgets field, ergo the types for e.g. workspace.save() is now not PerspectiveWorkspaceConfig (it's an anonymous struct type). Add the explicit returns types to save() will show this error.
| if (!newTables[t]) { | ||
| await viewer.eject(); | ||
| } else { | ||
| await viewer.load(newTables[t]); |
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.
Please batch these and await them all at the end using Promise.all(), so the user does not have to watch a slide show of these elements updating.
| async restore(value: PerspectiveWorkspaceConfig<string>) { | ||
| async restore(value: PerspectiveWorkspaceConfig) { | ||
| // bail out early if there is nothing to restore. | ||
| const layout = await this.save(); |
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.
Why is this necessary? This operation is not free and both perspective-viewer and lumino DockPanel should do this internally already.
|
|
||
| interface PerspectiveWorkspaceProps { | ||
| tables: Record<string, Promise<psp.Table>>; | ||
| layout: PerspectiveWorkspaceConfig; |
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.
This is called a config in the API docs, examples and the type name itself. Layout is the type of the layout field within this struct.
b60ab32 to
2401842
Compare
| if (viewer) { | ||
| widget = starting_widgets.find((x) => x.viewer === viewer); | ||
| if (widget) { | ||
| console.warn("DDD Restore load??? "); |
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.
stray debug print
| this.getAllWidgets().forEach((widget) => { | ||
| const psp_widget = widget as PerspectiveViewerWidget; | ||
| if (psp_widget.viewer.getAttribute("table") === name) { | ||
| console.warn("DDD Loading...?", table); |
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.
stray debug print
| const widget = new PerspectiveViewerWidget({ node, viewer }); | ||
| widget.task = (async () => { | ||
| if (table) { | ||
| console.warn("DDD Loading B?", table); |
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.
stray debug print
| @@ -0,0 +1,38 @@ | |||
| // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ | |||
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.
if this workspace-repro example is to repro a bug case, I imagine you want to remove this for the final PR
d9095a1 to
e65f89c
Compare
e65f89c to
95403a5
Compare
The ObservableMap's delete listener used to be able to "reject" deletions by returning false. This is no longer the case; the listener in Workspace always returns undefined now. Account for this in observable map's delete method. Co-authored by: Davis Silverman <[email protected]> Signed-off-by: Tom Jakubowski <[email protected]>
Signed-off-by: Davis Silverman <[email protected]>
95403a5 to
141ace2
Compare
Hello! This PR adds onto our
perspective-reactpackage an API for the<perspective-workspace>custom element in the form of a<PerspectiveWorkspace />React component. You can use it as such:You can see that this PR also adds some free functions (namely
addViewerandgenId) to assist in the functional style of React.There are also callbacks that can be passed for the
on-layout-updateandon-new-viewevents.The
PerspectiveWorkspaceConfiginterface was updated to better reflect the reality of the current workspace structure.During the course of this, I encountered a bug of an internal engine message not being swallowed by the engine. That bug is tracked in #3039.