-
Notifications
You must be signed in to change notification settings - Fork 4
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
Improve integration with Notebook 7 #16
Conversation
app.restored.then(() => { | ||
// use the JupyterLab command to open the file from the URL | ||
urls.forEach(url => { | ||
void commands.execute('filebrowser:open-url', { url }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, this drops the implicit dependency on the upstream command in JupyterLab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it would be worth reconsidering some time in the future to unify how files are fetched and opened in both JupyterLab and the different Notebook 7 applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's a good idea that we can maybe move faster here, and then upstream back?
}); | ||
|
||
// handle the route and remove the fromURL parameter | ||
const handleRoute = () => { | ||
const url = new URL(URLExt.join(PageConfig.getBaseUrl(), request)); | ||
// only remove the fromURL parameter | ||
url.searchParams.delete(paramName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I'd suggest (maybe optionally, but maybe not) not deleting the query param. At least for me, that means users can copy a jupyterlite link and give it to someone else, and it will actually 'just' work
Anything that can help move this forward? :D |
We can address your suggestions above, and then it should be good to go for a new release. |
Sounds great! I think this is ready to come out of draft and be merged / released? (I don't have rights in this repo) |
There are still a couple of rough edges, for example the file name not being shown properly in the URL after loading the file. And also since the file is downloaded, there is the popup asking to overwrite the file next time a user opens the same URL (which is technically expected, although could become annoying). But it's usable. So we could indeed merge, release, and fix the remaining issues later? |
I added you to the repo. We're using the Jupyter Releaser for making the releases. It's just a matter of triggering GitHub workflows: https://jupyter-releaser.readthedocs.io/en/latest/get_started/making_release_from_repo.html If you would like to give it a try, we can use |
Would this occur in jupyterlite even if you're just using the memory storage backend? |
yay for more rights :) Although I think in this case you shouldn't wait for me, just do the release when you feel it's right. For me, if this doesn't have the overewrite dialog show up in the memory backend, it's good to go :) |
Good point, it should indeed not store the file if enabled. We can add it to the ReadTheDocs demo deployment to check if it works as expected. |
|
For reference opening the same link looks fine after enabling the memory storage on the demo RTD deployment: open-url-parameter-jupyterlite-memory-storage.webm |
Fixes #14.
Opening a notebook
jupyterlab-open-url-paramater-notebook.webm
Open a file
jupyterlab-open-url-parameter-edit.webm