search

CDAT / jupyter-vcdat

A Jupyter Lab extension based on vCDAT.
BSD 3-Clause "New" or "Revised" License
6 stars 5 forks source link

Sidecar update to functionality #136

Closed downiec closed 5 years ago

downiec commented 5 years ago

Sidecar can be toggled off and on. Sidecar will be removed automatically if corresponding notebook is closed. Sidecar code is injected in notebook for users to see. When switching between notebooks, the corresponding sidecar is activated. Sidecars can no longer be closed manually (to prevent them from disappearing). Each notebook will have only one sidecar and it will display the last plot made in the sidecar.

downiec commented 5 years ago

This PR resolves issues #124 and #125.

sterlingbaldwin commented 5 years ago

All looks good to me now, thanks for the changes.

downiec commented 5 years ago

Great! Thanks for the suggestions!

sterlingbaldwin commented 5 years ago

I just merged this branch into my colormap branch so I could see how it worked in the real world. My one note on usability is that when you disable the sidecar close button you should hide it as well. It could be a bit confusing to have the little 'x' there but not have it do anything when the user clicks it.

Also, when the sidecar tab is collapsed and a new graphics method is selected, it suddenly pops open. Before this gets merged I would suggest three changes (these may require some research and work):

a) Hide the sidecare x button. b) Make the "plot to sidecare" switch open and close the sidecar tab. c) Make sure that the sidecar tab doesnt open when the "plot to sidecare" option is turned off.

downiec commented 5 years ago

I just merged this branch into my colormap branch so I could see how it worked in the real world. My one note on usability is that when you disable the sidecar close button you should hide it as well. It could be a bit confusing to have the little 'x' there but not have it do anything when the user clicks it.

Also, when the sidecar tab is collapsed and a new graphics method is selected, it suddenly pops open. Before this gets merged I would suggest three changes (these may require some research and work):

a) Hide the sidecare x button. b) Make the "plot to sidecare" switch open and close the sidecar tab. c) Make sure that the sidecar tab doesnt open when the "plot to sidecare" option is turned off.

I just want to mention that because of the way the sidecar is, I have to use little hacks and things to get it to behave the way I want. It doesn't have the interface to control them beyond some very basic things (so I have to directly access the node and maybe manipulate the DOM, which is not ideal). For example, I was trying to get the 'x' to disappear even before I made this PR, but it seems to not let you remove it, so I chose to fix that later and focus on more important functionality. I agree with all your usability suggestions and will try my best to get them done in time for release but I can't promise anything. I may have to make another PR to address these issues, thanks for the suggestions!

sterlingbaldwin commented 5 years ago

Ya they dont have the best API for interfacing with the widget, its pretty bare bones. It might be possible to call "hide" and "open" through the jupyter widget interface (not sure it is, just a thought).

downiec commented 5 years ago

I tried using the show and hide methods a while ago and what they do is show/hide the plot, but the tab stays open. That's why I had to resort to the activateById method to switch between sidecars. However yesterday I tapped into the ILabShell interface which lets me show and hide the shell panels. So I think I can get that all working. I'm still having a problem figuring out how to get rid of the 'x's though, and I was going to try to do a quick and dirty hack to delete the div that contains them, but getting to the div isn't straight forward. I think I'll leave the 'x' removal for another PR (it's not straight forward at all). I'm still working on this PR and will let you know when its ready for another review.

downiec commented 5 years ago

I will have to make another PR to handle the 'X' on the sidecar tabs, and I'm also planning on improving the logic so that notebooks can be renamed without causing them to 'disconnect' with their respective sidecar. Currently I track which sidecar belongs to which notebook by matching their name, which will cause issues if a notebook is renamed. You are welcome to review/retry the functionality now. I will be working to get it installing correctly on circleci next.

sterlingbaldwin commented 5 years ago

Very nice. Its opening and closing like it should, and when I switch between notebooks it switches to the correct sidecare tab. I think that once its building correctly in circleci that this gets merged.

downiec commented 5 years ago

Great! Sounds good. I noticed that when JupyterLab updated to 1.1, they changed the name of some icons/html elements. So I spent this morning updating the locators in the selenium tests and running some manual tests on my local Mac. The tests all passed!