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

add dialog checkbox to include token in URL #55

Merged
merged 4 commits into from
Mar 17, 2023

Conversation

minrk
Copy link
Contributor

@minrk minrk commented Feb 10, 2023

This could be considered a solution to #54, though it's the simplest possible one (no settings or server-side override).

  • adds checkbox to toggle whether share URLs have the token embedded. This is per-dialog and off by default, so to preserve current behavior requires an extra click, which feels appropriate to me given the consequences of sharing a link with credentials
  • Updates messages for with/without token and further clarification in JupyterHub context
  • only shows the checkbox if there's a token to be shared (e.g. in local jupyter with a password, there is no token to share)
  • There's a weird CSS jump as the dialog changes sizes when the message changes. I'm not sure how best to fix that.

There are lots of other ways to address #54 in terms of UX - where/when/who picks which link(s) to share. This is just one option.

In action:

Screen.Recording.2023-02-10.at.10.45.12.mov

Extra bit: I had to bump tsconfig target to es2018 to get it to build for some error with AsyncIterator. I'm not sure why since I didn't change any dependencies.

something about AsyncIterator
show different warnings under JupyterHub and not
@jtpio jtpio added the enhancement New feature or request label Feb 21, 2023
@jtpio
Copy link
Member

jtpio commented Mar 1, 2023

Thanks @minrk for this.

Looks like CI is failing due to a lint issue.

@jtpio
Copy link
Member

jtpio commented Mar 1, 2023

Extra bit: I had to bump tsconfig target to es2018 to get it to build for some error with AsyncIterator. I'm not sure why since I didn't change any dependencies.

Right this seems to be an issue with JupyterLab 3.6 (jupyterlab/jupyterlab#14029).

@minrk minrk force-pushed the include-token-checkbox branch from f40e881 to 7c39b3f Compare March 15, 2023 11:05
@minrk
Copy link
Contributor Author

minrk commented Mar 16, 2023

Sorry for dropping off. Fixed lint.

@jtpio
Copy link
Member

jtpio commented Mar 16, 2023

There's a weird CSS jump as the dialog changes sizes when the message changes. I'm not sure how best to fix that.

It's probably ok for now. We can always track this in a separate issue to iterate on it.

@jtpio
Copy link
Member

jtpio commented Mar 16, 2023

Just tried with the Binder for this PR: https://mybinder.org/v2/gh/minrk/jupyterlab-link-share/include-token-checkbox

And it looks like the token is not included after enabling the checkbox and clicking on Copy Link:

include-token-url.mp4

Although it shows up correctly in the dialog, probably just an issue when copying to the clipboard.

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit 631e930 into jupyterlab-contrib:main Mar 17, 2023
@minrk minrk deleted the include-token-checkbox branch March 17, 2023 10:32
@jtpio
Copy link
Member

jtpio commented Mar 17, 2023

New release with this change: https://github.com/jupyterlab-contrib/jupyterlab-link-share/releases/tag/v0.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants