-
Notifications
You must be signed in to change notification settings - Fork 174
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
More progress bar improvements (incl. Jupyter lab support) #1428
Conversation
I'll test this on my Windows machine, however note there's a few flake8 errors. |
Worked in both Jupyter Lab 0.32rc1 on Windows, as well as Jupyter Notebook 5.4.1. Fix the static checks and this LGTM. |
Fixed the static checks and added changelog entries. |
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.
LGTM. Feel free to merge once the tests pass.
FYI, this also worked on Jupyter Lab 0.32 on Ubuntu 17.10 |
fyi: This progress bar not only works in a notebook opened in Jupyter Lab, but also in a console opened in Jupyter Lab! 😃 |
I've been using this branch for more than a month now. It works on Ubuntu 18.04 too! Consequently, I strongly recommend this for merging. |
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.
LGTM; I'll fix the typo in the merge if you can let me know that it is indeed a typo @jgosmann.
nengo/utils/progress.py
Outdated
class VdomOrHtmlProgressBar(ProgressBar): | ||
"""Progress bar using the VDOM or HTML progress bar. | ||
|
||
This progress bar will transmit both representations as port of a MIME |
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.
Just to make sure, port
should be part
right?
You're right that should be "part".
…On August 30, 2018 12:45:36 PM EDT, Trevor Bekolay ***@***.***> wrote:
tbekolay approved this pull request.
LGTM; I'll fix the typo in the merge if you can let me know that it is
indeed a typo @jgosmann.
> @@ -399,18 +527,56 @@ def _update_unknown_steps(self, progress):
'''
+class VdomOrHtmlProgressBar(ProgressBar):
+ """Progress bar using the VDOM or HTML progress bar.
+
+ This progress bar will transmit both representations as port of a
MIME
Just to make sure, `port` should be `part` right?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1428 (review)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
Great thanks! |
e91b37b
to
f4a468f
Compare
f4a468f
to
a3162b0
Compare
Motivation and context:
Jupyter lab disables the execution of Javascript which breaks the current progress bar implementation. Version 0.32 (not released yet, but release candidates are available from PyPI) fixing an updating issue in the VDOM support that allows the implementation of the progress bar without any Javascript.
VdomProgressBar
takes advantage of that and works in Jupyter Lab, it might also work in future versions of Jupyter Notebook and would then allow the removal ofHtmlProgressBar
.As convenience I added a
VdomOrHtmlProgressBar
that provides both progress bars and the Jupyter client will select the preferred version (usually VDOM if VDOM is supported). This is the new default in clients the support IPythondisplay
.There is some sort of code duplication in
VdomProgressBar
andHtmlProgressBar
as they effectively both construct the same DOM. We could use the VDOM fromVdomProgressBar
and parse it to proper HTML for theHtmlProgressBar
. However, this might be wasted effort if theHtmlProgressBar
might get removed in the future. While it is not super complicated todo, the CSS attribute names need to converted from camel case to hyphen notation which might be mildly annoying. But I'm open to other opinions.Furthermore, this adds a warning that gets displayed in Jupyter Notebook versions before 5 (which are not supported).
Addresses #1426. Closes #1087.
Interactions with other PRs:
none
How has this been tested?
Manually tried in Jupyter Notebook 5.4.1 + 4.4.1, Jupyter Lab 0.32.0rc1.
How long should this take to review?
Where should a reviewer start?
I recommend going commit by commit as they are pretty independent.
Types of changes:
Checklist: