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

Update workspaces to automatically resolve. #6234

Merged
merged 4 commits into from Apr 27, 2019
Merged

Conversation

@afshin
Copy link
Member

@afshin afshin commented Apr 23, 2019

Fixes #6210

  • Removes the workspace from file browser "copy shareable link", returning a simple tree URL.
  • Makes resolve-workspace behavior automatic and removes the query string parameter and redirect dialog.
@afshin afshin added this to the 1.0 milestone Apr 23, 2019
@afshin afshin self-assigned this Apr 23, 2019
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Apr 23, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 23, 2019

#6210 explicitly mentions:

However, when I am trying to share this notebook to non-technical background ppl, it becomes a problem.

Perhaps one part of the problem is having the new workspace dialog, but perhaps another part is even having the mention of a workspace at all. My workspaces are likely quite different from your workspaces, so there is no reason for a link from me to include my workspace information. Instead, it seems to me that the link should elide the workspace information, as mentioned in #6210, and just have the /tree url. Thoughts?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 23, 2019

Just having the /tree url essentially is putting the file into a user's default workspace, which I think is simpler and more likely to satisfy the "non-technical background ppl", than opening a seemingly random workspace of yours that happens to be named the same as mine.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 23, 2019

CC also the OP: @bobmayuze

@afshin
Copy link
Member Author

@afshin afshin commented Apr 23, 2019

@jasongrout That is a reasonable solution. resolve-workspace is still broken, but maybe solving it in a different PR is a better move.

@afshin
Copy link
Member Author

@afshin afshin commented Apr 23, 2019

Actually, the resolve-workspace issues still need to happen here because even if we create just a /tree URL, that can collide with the default workspace, so adding the resolve-workspace param will fix that ... if it works.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 23, 2019

Actually, the resolve-workspace issues still need to happen here because even if we create just a /tree URL, that can collide with the default workspace, so adding the resolve-workspace param will fix that ... if it works.

True, good point.

@bobmayuze
Copy link
Member

@bobmayuze bobmayuze commented Apr 23, 2019

We could probably have something like a binder to host the notebooks statically? Such as having a Copy Static Shareable Link which will create a static file to serve the current notebook just like google docs ReadOnly shareable link? Other folks could reference this static page whenever they need to, but they won't be able to run any code in that way. One way to solve it is by adding a fork this notebook to my workspace or something like that.

@afshin
Copy link
Member Author

@afshin afshin commented Apr 23, 2019

@bobmayuze The copy shareable link functionality is intended as a lowest common denominator thing that doesn't necessarily require a public internet connection, it's meant to simply mimic what you can already do in the classic interface. What you are suggesting would be a good candidate for an extension, though.

@afshin
Copy link
Member Author

@afshin afshin commented Apr 24, 2019

@jasongrout @ian-r-rose This problem is making me realize you were right to suggest that resolve-workspace be the default behavior.

If we make all workspace resolution happen automatically we allow users to create new workspaces by typing the URL but always making sure they land somewhere useful if the URL they wanted is currently occupied. Then users would not need to know some special URL parameter to get the behavior they surely want, which is for the URL they entered to work.

@afshin
Copy link
Member Author

@afshin afshin commented Apr 24, 2019

We could remove that whole dialog, actually. That was UX scaffolding.

@afshin
Copy link
Member Author

@afshin afshin commented Apr 24, 2019

Right now, the risk is that the user gets annoyed by the persistent dialog frequently. Removing that annoyance changes the risk to user forgot a previous workspace's name. We can fix that by adding a help menu item that lists the saved sessions that are returned by the REST request GET /lab/api/workspaces.

Which of these should be in this PR? All three or some subset? My inclination is all three. But I'm also happy to break them out into separate pull requests.

This PR should:

  • Remove the workspace from file browser "copy shareable link", have it just be a tree URL.
  • Make resolve-workspace behavior automatic and remove the query string parameter.

In a separate PR:

  • Add list of active sessions in the UI somewhere.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 24, 2019

My inclination is all three

I'm happy to review all three in this PR, if it's easier for you.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 24, 2019

Add list of active sessions to help menu.

Perhaps it should be a section in the Tabs menu? Or a new Workspaces menu? It seems a little weird to me it would be in the Help menu.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 24, 2019

Hmmm...maybe it would be better to do "Add list of active sessions to help menu." in a different PR, since it involves at least some UX consideration about where to put that list.

@afshin
Copy link
Member Author

@afshin afshin commented Apr 24, 2019

Okay good plan. Let's have the first two in this PR.

@afshin afshin changed the title [WIP] Guarantee "Copy Shareable Link" produces a link that always works. Guarantee "Copy Shareable Link" produces a link that always works. Apr 24, 2019
@afshin afshin changed the title Guarantee "Copy Shareable Link" produces a link that always works. Update workspace URL behavior to automatically resolve. Apr 24, 2019
@afshin afshin changed the title Update workspace URL behavior to automatically resolve. Update workspaces to automatically resolve. Apr 24, 2019
@afshin afshin requested a review from jasongrout Apr 24, 2019
Copy link
Member

@blink1073 blink1073 left a comment

Looks good, thanks!

@blink1073 blink1073 merged commit e3bfff3 into jupyterlab:master Apr 27, 2019
9 checks passed
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 27, 2019

ping @mpacer

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants