Add variables panel to side view (variables control currently empty) - RESUBMIT #404
Conversation
|
Bleh, I know that @rchiodo looked at this already on the old PR. Rich, I've not addressed your comment yet, but I will. I have a bunch of comments on this PR in my previous PR here: #401 so check that out for more info and comments on the work., But I created that PR off of my fork, so it wasn't running the PR validation steps. This is the same PR, but pushed to upstream and not to my fork. |
IanMatthewHuff
changed the title
|
Does this PR move the variable window to the side panel for existing notebook and interactive window as well? |
|
@DonJayamanne Nothing in the plan currently to move the existing variable explorers. Second PR here is adding the react control and hooking it up to active Notebook. |
|
@DonJayamanne I know that we're both adding viewContainers, but seems like that should be pretty easy to merge as each view is independent. |
| public [VariableViewMessages.Started]: never | undefined; | ||
| public [VariableViewMessages.UpdateSettings]: string; | ||
| public [CssMessages.GetCssRequest]: IGetCssRequest; | ||
| public [CssMessages.GetCssResponse]: IGetCssResponse; |
There was a problem hiding this comment.
Can't we use existing messages?
I thought some of these were shared messages.
There was a problem hiding this comment.
@DonJayamanne I resolved two of your comments above. This comment and the comment below it regarding the message classes I'd like to save for a later code review. I think your comments are legit, but this is currently how the message classes are constructed for all our viewer classes (DataViewer, PlotViewer, ect..) so if I change this I'd like to fix them all up in a separate PR. To not forget this I tagged the info in the bug for variable viewer (which I'm not resolving yet). Changes might need to do a bit more as making the I*Mapping classes properties static creates TS errors for using a class at all (versus a namespace) and I'm not fully sure if the PostOffice code is ok with that.
That ok?
|
|
||
| // Map all messages to specific payloads | ||
| export class IVariableViewMapping { | ||
| public [VariableViewMessages.Started]: never | undefined; |
There was a problem hiding this comment.
Why not make these static properties, else we'd need to new up this class everytime.
For #250
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).