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

Create tree plugin #1374

Merged
merged 11 commits into from Aug 10, 2023
Merged

Create tree plugin #1374

merged 11 commits into from Aug 10, 2023

Conversation

trungleduc
Copy link
Member

@trungleduc trungleduc commented Aug 8, 2023

References

Closes #1372

tree

Code changes

  • pyzmq is pinned to 25.1.0 on the Mac OS pipeline because 25.1.1 is not available as wheel yet on Mac OS

User-facing changes

  • The default tree page of Voila is now using the file browser widget of JupyterLab
  • The jinja-based tree page is still supported, but users need to activate it with the --classic-tree CLI option, the VoilaConfiguration.classic_tree config or ?classic-tree=True in the query string.
  • JupyterLab custom theme is supported with the new tree page, for the classic tree page, only the light and dark theme are supported.

Backwards-incompatible changes

  • Users need to activate the classic tree to use existing tree templates.

@trungleduc trungleduc added the enhancement New feature or request label Aug 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

Binder 👈 Launch a Binder on branch trungleduc/voila/tree-page

@trungleduc trungleduc added this to the 0.5.0 milestone Aug 8, 2023
@jtpio
Copy link
Member

jtpio commented Aug 9, 2023

@trungleduc you mentioned offline that interacting with the file browser items in this PR only requires 1 click, instead of the double click like in JupyterLab?

There is an issue to add this behavior in JupyterLab so that it could be configured, which would be especially useful for Notebook 7: jupyterlab/jupyterlab#14071

Do you think this could be implemented upstream? This could go in JupyterLab 4.1 as a new feature.

@trungleduc
Copy link
Member Author

@trungleduc you mentioned offline that interacting with the file browser items in this PR only requires 1 click, instead of the double click like in JupyterLab?

There is an issue to add this behavior in JupyterLab so that it could be configured, which would be especially useful for Notebook 7: jupyterlab/jupyterlab#14071

Do you think this could be implemented upstream? This could go in JupyterLab 4.1 as a new feature.

I was able to implement this feature since in the case of voila, I don't need other interactions like multiple selections or drag/drop. So I just need to overwrite the single click handler and disable all others.

Upstreaming this is not a technical problem but I think we need more UX studies before adding the single click behavior to Lab.

}

handleEvent(event: Event): void {
if (event.type === 'click') {
Copy link
Member

Choose a reason for hiding this comment

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

@jtpio
Copy link
Member

jtpio commented Aug 9, 2023

Upstreaming this is not a technical problem but I think we need more UX studies before adding the single click behavior to Lab.

Maybe it would be fine if it is behind a user setting (opt-in)?

@trungleduc
Copy link
Member Author

Upstreaming this is not a technical problem but I think we need more UX studies before adding the single click behavior to Lab.

Maybe it would be fine if it is behind a user setting (opt-in)?

Indeed, I've just realized that we don't have any advanced interactions (drag/drop, ...) in the Notebook v6 browser. I'll open a PR after finishing this one.

I'll keep the implementation here to release Voila ASAP and will reuse the upstream version once it's released.

@jtpio
Copy link
Member

jtpio commented Aug 9, 2023

Indeed, I've just realized that we don't have any advanced interactions (drag/drop, ...) in the Notebook v6 browser. I'll open a PR after finishing this one.

Yes, and in Notebook 7 we can use the checkboxes to select multiple items.

I'll keep the implementation here to release Voila ASAP and will reuse the upstream version once it's released.

Makes sense, it might take a little while to get this released in JupyterLab.

Thanks!

@trungleduc
Copy link
Member Author

Please update galata references

@trungleduc trungleduc closed this Aug 9, 2023
@trungleduc trungleduc reopened this Aug 9, 2023
voila/app.py Outdated Show resolved Hide resolved
voila/treehandler.py Outdated Show resolved Hide resolved
trungleduc and others added 3 commits August 10, 2023 10:36
Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
@trungleduc
Copy link
Member Author

Please update galata references

@trungleduc trungleduc marked this pull request as ready for review August 10, 2023 10:17
@trungleduc
Copy link
Member Author

@jtpio could you re-take a look at this PR? I will release RC after this PR and update the documentation after that.

@jtpio jtpio self-requested a review August 10, 2023 12:18
@@ -84,7 +84,8 @@ jobs:

- name: Create the conda environment
shell: bash -l {0}
run: mamba install -q python=${{ matrix.python_version }} pip jupyterlab_pygments==0.1.0 pytest-cov pytest-rerunfailures nodejs=18 yarn ipywidgets matplotlib xeus-cling "traitlets>=5.0.3,<6" ipykernel
# TODO unpin pyzmq
Copy link
Member

Choose a reason for hiding this comment

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

What's the issue with pyzmq?

Copy link
Member Author

Choose a reason for hiding this comment

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

The 25.1.1 is not available as wheel yet on Mac OS

Copy link
Member

Choose a reason for hiding this comment

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

ah ok, maybe mentioning this in the comment could be useful for later when we get back to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

I meant next to the TODO, or in a new issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

voila/configuration.py Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented Aug 10, 2023

update the documentation after that.

Nice. Maybe a note in the changelog (as a highlight) would also be useful (but could also be done later).

edit: opened #1377

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
voila/app.py Outdated
@@ -671,6 +680,13 @@ def init_handlers(self) -> List:
"no_cache_paths": ["/"],
},
),
(
url_path_join(
self.server_url, r"/voila/api/contents%s" % path_regex
Copy link
Member

Choose a reason for hiding this comment

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

Curious: could exposing this endpoint allow someone access the content of other files on disk when they are not supposed to? For example when starting voila with voila basics.ipynb and trying to access another notebook via this endpoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed this endpoint is leaking the file structure (not the content) of the current working directory in the case of starting Voila with one notebook, I'll fix it.

Copy link
Member

Choose a reason for hiding this comment

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

ok thanks for confirming 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by 9c48666

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.

Looks good, thanks!

And CI all green.

@trungleduc trungleduc merged commit 8a46a25 into voila-dashboards:main Aug 10, 2023
25 checks passed
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.

More advanced tree view
2 participants