-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Left right panel in Notebook view #6487
Conversation
Nice thanks @brichet for picking this up 👍 |
bda4cf8
to
a6b41a0
Compare
Is there a way to retrieve the |
hmm strange, if it changes during the check release then it should also change locally after running |
Right, sorry for that, indeed after cleaning everything it changes |
7589f6c
to
b885e7c
Compare
Maybe we could increase the test time. This PR adds some little ui-test, but I think 10 minutest is too short. |
Hmm normally 10 minutes should be more than enough? Or do the new tests require that much more time? Here is a screenshot of the cancelled check: https://github.com/jupyter/notebook/runs/7686750791?check_suite_focus=true But on |
Not that much, they should be really quick, only opening left (toc) and right (notebooktools) panels. |
Maybe there is something that makes them time out on CI (normally locally as well) with the new changes? |
The timeout is quite long, 4 minutes :
When a test fail 2 times (it is retried 1 time) while awaiting a specific screenshot or element, it almost timeout the whole tests. Is there a reason to have a 4 minutes timeout ? Galata is set to 1 minute by default. That will not fix the tests but may allow us to get the errors instead of cancelled test. |
This was added in jupyterlab/retrolab#273, but I don't see any reason for that particular value. So we can change it and see. |
packages/application/src/app.ts
Outdated
this.restored = this.shell.restored | ||
.then(() => undefined) | ||
.catch(() => undefined); |
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.
Is this needed? Or could this.shell.restored.then()
be directly used on the line below?
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.
Or maybe this.restored = this.shell.restored
?
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.
Right, it's left from a copy and paste.
But we should keep this.restored = this.shell.restored
. For example, the table of contents extension waits for app
to be restored, but needs the Notebook panel to be ready. That's why the shell
resolves the restored only when the main area has been added.
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.
Fixed in 52f10ed
Good catch |
The errors on tests seems not related (save status, kernel status...), maybe we can try running them again. |
Yes these are a bit flaky sometimes. |
Just tried on Binder and it looks good. left-right-areas-shift.mp4At first it might feel a bit odd that the notebook is shifted to the side when the left or right area is opened. @hbcarlos had some good feedback about this on the original PR: jupyterlab/retrolab#275 (comment) Posting it here for reference:
Probably we could try to change how the side panels are positioned in the For reference Google Docs does shift the main page a bit when opening an add-on on the right side: google-docs-right-area.mp4Although their top area takes the full width, which reduces this feel of being "off" since the page is still centered with the top area. |
Nice, thanks for sharing, it would be better that way. |
The debugger panel is also missing, but I think it can be added in a follow up PR as well. I'm not sure it will be as easy to include than the other panels, as it depend on current opened widget ( |
Agree this can be added in a different PR 👍 Hopefully the extension should already handle the active widget properly, otherwise we can submit fixes upstream in JupyterLab. |
FYI @brichet I'm planning to do a pass on the changes, especially the ones on the shell. Overall it looks good. I would like to experiment with the alternative side-panel approach mentioned above. Will report here when I have more updates. |
Thanks @jtpio. |
f679f3e
to
a6717a6
Compare
Fix order of selectors
a6717a6
to
708928b
Compare
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.
Nice thanks @brichet for the hard work and your patience!
We can track follow-up improvements and fixes discussed in the PR in separate issues.
This PR follows #6327 to add left and right panel to Notebook.
The left and right panels can be opened from the
view
menu.Closes #6327
Fixes #6326
Fixes #6403
Fixes #6405