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

Upgrade ipywidgets to v7, with a fallback for v6 #738

Merged
merged 7 commits into from
Nov 17, 2017
Merged

Upgrade ipywidgets to v7, with a fallback for v6 #738

merged 7 commits into from
Nov 17, 2017

Conversation

scottdraves
Copy link
Contributor

fix #717

@scottdraves
Copy link
Contributor Author

@jasongrout

@bollwyvl
Copy link
Contributor

bollwyvl commented Nov 7, 2017

I'd love to see this make it in but do wonder about our ongoing ability to maintain it and/or extend to other mime renderers. If there's any way that we can make future modifications one-liners to a single (json) file, that would be preferable.

At a minimum, I'd like to see the unpkg urls moved to something configurable...

@jasongrout
Copy link
Member

The code looks okay to me. @bollwyvl - do you have any tips on how to make that URL configurable?

@MariuszJurowicz
Copy link

@jasongrout @bollwyvl any updates here?

@jasongrout
Copy link
Member

I think I figured out how to make it configurable. Basically, look in https://github.com/jupyter/nbviewer/blob/master/nbviewer/providers/base.py and https://github.com/jupyter/nbviewer/blob/master/nbviewer/app.py for "mathjax_url" and do something similar for the ipywidgets url (you'll probably have two ipywidgets urls, one for v6, one for v7).

@jasongrout
Copy link
Member

If we want to do this configuration in a separate PR, I'm okay with that as well. This PR is good and self-contained as an upgrade as-is, though I agree with @bollwyvl that having a configurable URL would be nice.

@jasongrout
Copy link
Member

In fact, if you've tested this PR as-is, I'd be inclined to merge this as-is and then we can do the configuration changes in a separate PR. Let me know if you want to do the configuration in this one, though.

@jasongrout
Copy link
Member

Actually, looking more into the source code for nbviewer, I do have one more comment on this PR. As-is, the widgets code is added in the layout.html template, but I think that's the wrong place (and it was wrong before - not your fault!). The layout template is used for things like the page listing all of the notebooks, etc. Instead the widgets code should be added to the https://github.com/jupyter/nbviewer/blob/master/nbviewer/templates/notebook.html template, right next to the mathjax block at

{% block mathjax %}
. Then it will be loaded just for actual notebooks. Can you make this change in this PR?

And please let us know if you want to do the configuration changes in this PR, or if we should open an issue/PR for it separately.

@bollwyvl
Copy link
Contributor

bollwyvl commented Nov 10, 2017

Hi, sorry for the delay. It's absolutely good to go, as we want widgets :)

I guess there are two dimensions of configuration:

  • what mimetypes do we support?
  • where do we get them?

So the easiest answer from the downstream perspective would be that the mimetype itself would carry enough information to determine what we need to render it. Having to pre-parse the content that will get re-parsed again seems pretty rough... what if the json is bad?

Then we'd need a simple file someplace, probably realizable in the config file, or at least pointed to by the config file.

This could then either be requested with $.get and used directly, or, saving a step, generate some boilerplate.

Anyhow... I boiled all this down to some ideas here:

https://nbviewer.jupyter.org/gist/anonymous/a44bf8d688135d58588c07dd61ec90ee

@bollwyvl
Copy link
Contributor

In fact, if you've tested this PR as-is,

Have not 😊

mathjax_url

Right, this pattern, but have a plan for something more general and replaceable. If the path of justice is unpkg, and it's possible to manually dereference this locally to be hosted disconnected from the internet, huzzah! unpkg is by neccessity not very policed, (vs cdnjs, jsdelivr, etc) and in fact has been used as a C&C for exploits. I wouldn't be surprised if it isn't blacklisted in a few places already.

loaded just for actual notebooks

Yes, this. Also, probably go ahead and wrap it in a {% block renderers %}{% endblock %}. At very least, a local admin could override the template and just kill that block.

@jasongrout
Copy link
Member

Note that jsdelivr also supports the 'anything that is an npm package is available' - we've looked at switching to it.

@scottdraves
Copy link
Contributor Author

thanks for the comments.
moving from layout.html to notebook.html should be no problem, that sounds better.
look for a commit on monday.
making it more configurable sounds like a new issue...

@jasongrout
Copy link
Member

thanks for the comments.
moving from layout.html to notebook.html should be no problem, that sounds better.
look for a commit on monday.

Great, thanks!

making it more configurable sounds like a new issue...

Sounds good! I made an issue: #739.

@bollwyvl
Copy link
Contributor

Had a chance to try this out locally: looking good!

The updates are looking robust, but more byzantine: I'd like us to start thinking about this in a more systematic way that will permit handling the anticipated explosion of mime renderers. Anyhow, out of scope for this issue, I suppose.

One regression I'm seeing is that (on chrome, but not FF), the imported material design stuff is changing some site chrome, initially have noticed link colors:
screen shot 2017-11-13 at 9 54 45 am

vs 6.x

screen shot 2017-11-13 at 9 55 25 am

This is from an eager un-namespaced a CSS selector with a freaky vendor selector:
screen shot 2017-11-13 at 9 56 35 am

We can either get more aggressive on this in the nbviewer selectors, but it might be a good thing to push upstream... not sure if it's worth holding up the

@scottdraves
Copy link
Contributor Author

scottdraves commented Nov 14, 2017

Thanks for the detailed testing.

This is a big blocker for us and i think the CSS fix lies elsewhere...

@scottdraves
Copy link
Contributor Author

ping...

@jasongrout
Copy link
Member

Thanks for the ping - it's in my queue to look at today.

@jasongrout
Copy link
Member

@bollwyvl - do you know where that a css styling is coming from? Is it coming from widgets?

@jasongrout
Copy link
Member

I reproduced the css styling problem, and tracked it down to the widgets embed script. I'll be solving that upstream.

I think this code looks good. I probably would have used jupyterwidgets_base_url or npm_base_url, but that can be a different PR.

@jasongrout
Copy link
Member

I also confirmed this works great for ipywidgets 6.

Thanks again!

@jasongrout jasongrout merged commit 67ee47e into jupyter:master Nov 17, 2017
@scottdraves
Copy link
Contributor Author

🐬 🐬

@jasongrout
Copy link
Member

The blue link issue is traced back to JupyterLab: jupyterlab/jupyterlab#3257

@scottdraves
Copy link
Contributor Author

when will this be deployed to the live service http://nbviewer.jupyter.org/ ?
is there a place to file an issue for that?

@jasongrout
Copy link
Member

Great question. I don't know how deployment works for nbviewer.

@jasongrout
Copy link
Member

Let's ask on the jupyterhub gitter channel.

@scottdraves
Copy link
Contributor Author

looks like it happened.

@jasongrout
Copy link
Member

Great! And we're working on fixing that CSS issue upstream, which should show up when we push an updated jupyterlab and ipywidgets.

@minrk
Copy link
Member

minrk commented Dec 6, 2017

This should be deployed to nbviewer. Sorry for taking so long. @jasongrout I've added you on the private nbviewer-deploy repo, so you should be able to do a deploy if you want.

@jasongrout
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Jupyter widgets 7.0 update
5 participants