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.

Sign up for GitHub

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

Jump to bottom

fix: use jsdelivr CDN for ipywidgets #491

Merged
merged 6 commits into from
Apr 13, 2023
Merged

fix: use jsdelivr CDN for ipywidgets #491

merged 6 commits into from
Apr 13, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Apr 13, 2023 •
edited by choldgraf
Loading

Should fix #458, supercedes #469

@agoose77 agoose77 requested a review from choldgraf April 13, 2023 10:44
@agoose77
Copy link
Collaborator Author

@choldgraf let's see the rendered PR docs, and judge if this did indeed work!

@codecov
Copy link

codecov bot commented Apr 13, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (5e7acb3) 81.47% compared to head (2deaa40) 81.47%.

❗ Current head 2deaa40 differs from pull request most recent head e6a1fee. Consider uploading reports for the commit e6a1fee to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #491   +/-   ##
=======================================
  Coverage   81.47%   81.47%           
=======================================
  Files          29       29           
  Lines        2618     2618           
=======================================
  Hits         2133     2133           
  Misses        485      485
Flag Coverage Δ
pytests 81.47% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
myst_nb/core/config.py 78.04% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

choldgraf
choldgraf previously approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

I believe that it works! Good catch:

myst_nb/core/config.py
@@ -64,7 +64,7 @@ def ipywidgets_js_factory() -> Dict[str, Dict[str, str]]:
"crossorigin": "anonymous",
},
# Load IPywidgets bundle for embedding.
"https://unpkg.com/@jupyter-widgets/html-manager@^0.20.0/dist/embed-amd.js": {
"https://cdn.jsdelivr.net/npm/@jupyter-widgets/html-manager@1.0.6/dist/embed-amd.js": {
Copy link
Member

@choldgraf choldgraf Apr 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

can we add a comment here that provides a loose guidelines for when/how to update this import link? Or maybe just a block comment at the top of the #Load IPywidgets.. section? doesn't need to block if you don't have the time, but I feel like you brought some valuable domain-knowledge to this fix that could be useful for others

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hehe, we're on the same wavelength: 2deaa40

@choldgraf choldgraf changed the title fix: use jsdelivr CDN fix: use jsdelivr CDN for ipywidgets Apr 13, 2023
@agoose77
Copy link
Collaborator Author

@choldgraf merge if you're happy!

choldgraf
choldgraf approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@choldgraf choldgraf merged commit 432d7bc into master Apr 13, 2023
@kolibril13 kolibril13 mentioned this pull request May 31, 2023
Closed
agoose77 added a commit to aleivag/MyST-NB that referenced this pull request Jun 23, 2023
@agoose77
fix: use jsdelivr CDN for ipywidgets (executablebooks#491)
09ff078 
* fix: use jsdelivr CDN

* chore: bump ipywidgets version to 8.0

* docs: add note about ipywidgets
@bsipocz bsipocz deleted the agoose77/fix-add-ipywidgets-cdn branch September 25, 2024 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@choldgraf choldgraf choldgraf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ipywidgets no longer rendering
2 participants
@agoose77 @choldgraf