Skip to content
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

[Jupyter] Tabs (lumino) #9696

Merged
merged 9 commits into from Feb 2, 2024
Merged

[Jupyter] Tabs (lumino) #9696

merged 9 commits into from Feb 2, 2024

Conversation

bbonenfant
Copy link
Contributor

@bbonenfant bbonenfant commented Jan 30, 2024

This implements a Tab layout of the extension using some builtin lumino functionality. This shifts us away from a UI model that that can be summarized as "explore view with a bunch of child views which we manually show and hide" to "views split between tabs that the user can 'freely' navigate between". To minimize the amount of changes included in this PR, we still have some situations where we force the user into certain views, such as:

  • when the user is not connected to/logged into a cluster they are brought to the config screen,
  • if a server error happens we show a full screen error.

To accomplish this I had to make the following changes:

  • Move the "explore" view into a component,
  • Remove all of the showConfig/showDatum/showPipeline/etc. flags and signals as they are no longer needed,
  • Collapsed the Pipeline and PipelineSplash screens into the same react component,

Some additional notes:

  • In order to restrict the user from navigating away from "forced" screens, we manually have to hide and show the tab bar. I don't think this particularly user friendly but this is maintaining functionality that already existed.
  • To get the "full screen error" to work in this setup, we add it as a tab and then remove the tab header. Again, I don't think an inescapable full screen error is user friendly but this is to maintain consistency.

Screen Recording:
https://github.com/pachyderm/pachyderm/assets/28938409/caee8215-c041-4073-8d60-ca54abeaf8ab

Copy link
Contributor

@emmajsadams emmajsadams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Really like this approach compared to the prior SplitPanel model. Just a few comments mostly some questions for learning purposes.

jupyter-extension/src/plugins/mount/mount.tsx Show resolved Hide resolved
jupyter-extension/src/plugins/mount/mount.tsx Outdated Show resolved Hide resolved
jupyter-extension/src/plugins/mount/mount.tsx Show resolved Hide resolved
jupyter-extension/src/plugins/mount/mount.tsx Outdated Show resolved Hide resolved
@bbonenfant bbonenfant marked this pull request as ready for review February 1, 2024 00:48
Copy link
Contributor

@emmajsadams emmajsadams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@smalyala
Copy link
Contributor

smalyala commented Feb 1, 2024

Minor nit: If successfully connected to a cluster and then I try to connect to an invalid cluster, the connect request fails and the 3 tabs at the top disappear which is great. The screen then prompts to connect to a cluster. If I click the "X", it appears as if I'm connected to this invalid cluster. It might make sense to remove that "X" if the user isn't currently connected to a cluster. Not a serious issue but it also occurs if my config file is pointing to an invalid cluster when I launch the extension. I might not know the cluster is invalid and it's not clear from the UI config page. I might think, "Hey, it says I'm connected to this cluster at localhost:8091, but nothing's happening. What gives?"

image

@bbonenfant
Copy link
Contributor Author

Minor nit: If successfully connected to a cluster and then I try to connect to an invalid cluster, the connect request fails and the 3 tabs at the top disappear which is great. The screen then prompts to connect to a cluster. If I click the "X", it appears as if I'm connected to this invalid cluster. It might make sense to remove that "X" if the user isn't currently connected to a cluster. Not a serious issue but it also occurs if my config file is pointing to an invalid cluster when I launch the extension. I might not know the cluster is invalid and it's not clear from the UI config page. I might think, "Hey, it says I'm connected to this cluster at localhost:8091, but nothing's happening. What gives?"
image

Good find. I was able to reproduce this off of master, so I don't think this is a new addition from these changes. I created a ticket for that here. I also notice that when you enter an invalid pachd address you get an error message and the field goes blank. That's also not new but should be fixed as well

@bbonenfant bbonenfant merged commit ab0f32b into master Feb 2, 2024
12 checks passed
@bbonenfant bbonenfant deleted the bonenfant/INT-927 branch February 2, 2024 18:39
bbonenfant added a commit that referenced this pull request Feb 22, 2024
This implements a Tab layout of the extension using some builtin
`lumino` functionality. This shifts us away from a UI model that that
can be summarized as "explore view with a bunch of child views which we
manually show and hide" to "views split between tabs that the user can
'freely' navigate between". To minimize the amount of changes included
in this PR, we still have some situations where we force the user into
certain views, such as:
* when the user is not connected to/logged into a cluster they are
brought to the config screen,
* if a server error happens we show a full screen error.

To accomplish this I had to make the following changes:
* Move the "explore" view into a component,
* Remove all of the showConfig/showDatum/showPipeline/etc. flags and
signals as they are no longer needed,
* Collapsed the Pipeline and PipelineSplash screens into the same react
component,
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants