- Notifications
You must be signed in to change notification settings - Fork1.3k
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 ourterms of service andprivacy 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 tob60ab32Compare| 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.
UseReact.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 importthemes.css for viewers within a workspace - use the specific themepro.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 usedthemes.css as well though so I removed that line.
| awaitnewPromise((r)=>setTimeout(r,delay)); | ||
| } | ||
| returnfalse; | ||
| } |
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 handlefor 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
| }; | ||
| } | ||
| exportfunctiongenId(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.
| constexists=document.querySelector("body > perspective-indicator"); | ||
| if(exists){ | ||
| returnexistsasHTMLElement; | ||
| } |
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.
| } | ||
| exportinterfacePerspectiveWorkspaceConfig<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'sDockPanel.saveLayout() does not return string identifiers forwidgets field, ergo the types for e.g.workspace.save() is now notPerspectiveWorkspaceConfig (it's an anonymous struct type). Add the explicit returns types tosave() will show this error.
| if(!newTables[t]){ | ||
| awaitviewer.eject(); | ||
| }else{ | ||
| awaitviewer.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 usingPromise.all(), so the user does not have to watch a slide show of these elements updating.
| asyncrestore(value:PerspectiveWorkspaceConfig<string>){ | ||
| asyncrestore(value:PerspectiveWorkspaceConfig){ | ||
| // bail out early if there is nothing to restore. | ||
| constlayout=awaitthis.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 bothperspective-viewer and luminoDockPanel should do this internally already.
| interfacePerspectiveWorkspaceProps{ | ||
| 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 aconfig in the API docs, examples and the type name itself.Layout is the type of thelayout field within this struct.
b60ab32 to2401842Compare| 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)=>{ | ||
| constpsp_widget=widgetasPerspectiveViewerWidget; | ||
| 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
| constwidget=newPerspectiveViewerWidget({ 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 @@ | |||
| // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ | |||
tomjakubowskiAug 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 toe65f89cComparee65f89c to95403a5CompareThe ObservableMap's delete listener used to be able to "reject"deletions by returning false. This is no longer the case; the listenerin Workspace always returns undefined now. Account for this inobservable map's delete method.Co-authored by: Davis Silverman <davis@prospective.dev>Signed-off-by: Tom Jakubowski <tom@prospective.dev>
Signed-off-by: Davis Silverman <davis@thedav.is>
95403a5 to141ace2Compare
Uh oh!
There was an error while loading.Please reload this page.
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.