Expose API to show DS Data Viewer component from external extension #12045
Conversation
|
|
||
| // Load the web panel using our current directory as we don't expect to load any other files | ||
| super.loadWebPanel(process.cwd()).catch(traceError); | ||
| private static trimTitle(title: string): string { |
There was a problem hiding this comment.
I'd let the UI (VSCode or webview) handle long titles (I.e. I'd remove this).
There was a problem hiding this comment.
Makes sense, it would be better for webview or VSCode to handle this.
Do you know how likely is to have long titles? I mention it because originally Data Viewer is trimming the titles, looking at the code and doing some testing removing this will show long names in the tabs (tested up to 200 chars), not sure if this could represent a regression for some users?
There was a problem hiding this comment.
@rchiodo @IanMatthewHuff @DavidKutu What do you think?
There was a problem hiding this comment.
The dataviewer name is based off the variable, right? Pretty sure we put this in as longer variable names could eat up like half of your tab real estate as VSCode didn't do anything to shorten them. Speaking for myself, I'd go for just letting the long names through. Let VSCode deal with it and don't have to own any custom trimming. The argument against is that customer are not picking variable names the same way they pick file names and might not be expecting to have so big of a tab.
There was a problem hiding this comment.
I don't think this would be the right spot though in any case. Some consumers might want to trim, others might not. They should be in charge of passing in the right length title that they want. I would think that the viewer should just show it.
There was a problem hiding this comment.
@IanMatthewHuff agree, I will remove the trimming from DataViewer and if the decision is to still trim titles we can do it from the client.
There was a problem hiding this comment.
Speaking for myself, I'd go for just letting the long names through. Let VSCode deal with it and don't have to own any custom trimming.
Agreed.
| // Fill in our variable's beginning data | ||
| this.variable = await this.prepVariable(variable, notebook); | ||
| // Load the web panel using our current directory as we don't expect to load any other files | ||
| await super.loadWebPanel(process.cwd()).catch(traceError); |
There was a problem hiding this comment.
Why are we swallowing errors here. Not sure this is the right thing to do.
How about changing from process.cwd to dataExplorereDir, makes it future proof as well, if we wanted to load soemthing.
There was a problem hiding this comment.
@DonJayamanne great comments :) thanks
-
Error swallowing: hmm I think it is because this call was done from the constructor, I moved here to make sure the webpanel is loaded before calling setTitle and show. I never realized the error swallowing though, will remove it.
-
Directory: Good point, just wondering if there is another reason why it was done like that. @rchiodo @IanMatthewHuff any thoughts?
src/client/datascience/data-viewing/jupyterVariableDataProvider.ts
Outdated
Show resolved
Hide resolved
src/client/datascience/data-viewing/jupyterVariableDataProvider.ts
Outdated
Show resolved
Hide resolved
rchiodo
requested review from
DavidKutu,
IanMatthewHuff,
joyceerhl,
rchiodo and
techwithtim
| request.variable, | ||
| this._notebook! | ||
| ); | ||
| const title: string = `${localize.DataScience.dataExplorerTitle()} - ${request.variable.name}`; |
There was a problem hiding this comment.
If we want to keep the title trimming in (my vote is no) this would be where we would want to do it I think.
IanMatthewHuff
left a comment
IanMatthewHuff
left a comment
There was a problem hiding this comment.
@sevillal A couple of cleanup items, but really good work with this :). Glad to see some code sharing between the teams, and I appreciate you walking us through the architecture before the PR, was much easier to review that way.
1. Do not trim title in DataViewer
2. More strucuted type to represent rows responses from IDataViewerDataProvider. Created IRowsResponse type {data: any[]}
3. Change createion of JupyterVariableDataProvider through a Factory Class
4. Fix initialization lock/flag in jupyterVariableDataProvider to ensure only one execution
5. showDataViewer API return void
6. Cache dataFrameInfo in DataViewer to prevent multiple fetches
7. Fix automotter change in reactSlickGrid.txt imports
8. Move IDataViewer and IDataViewerFactory to data-viewing/types
|
Kudos, SonarCloud Quality Gate passed!
|
|
@karthiknadig This is Sergio from our team, so removing external contributor tag. Wasn't sure if you recognized his github name. |
|
@sevillal I added the External Contributor on this by mistake. |
|
@rchiodo I don't have write permission in this repo and I am unable to merge (or edit the PR). I have asked for permission earlier today and waiting for it to be granted in order to merge :). |
|
No problem. Merged :) |
5 participants
For #12033
Has a news entry file (remember to thank yourself!).Has telemetry for enhancements.Unit tests & system/integration tests are added/updated.Test plan is updated as appropriate.package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).The wiki is updated with any design decisions/details.