Skip to content

add BeakerX tables to widget demos #239

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

Merged
merged 9 commits into from
Oct 25, 2017
Merged

add BeakerX tables to widget demos #239

merged 9 commits into from
Oct 25, 2017

Conversation

scottdraves
Copy link
Contributor

atn @jasongrout

note that the 1st pre-existing example (ipyleaflet) is broken: https://github.com/ellisonbg/ipyleaflet/issues/76

i think we know how to fix it, will look further into it next.

@scottdraves
Copy link
Contributor Author

screenshot:
screen shot 2017-10-16 at 10 10 33 am

@scottdraves
Copy link
Contributor Author

scottdraves commented Oct 20, 2017

This PR now fixes the ipyleaflet and pythreejs demos as well as adding the beakerx demo.
screen shot 2017-10-20 at 10 14 52 am
screen shot 2017-10-20 at 10 14 58 am

@Carreau Carreau requested a review from SylvainCorlay October 20, 2017 18:09
@Carreau
Copy link
Member

Carreau commented Oct 20, 2017

cc @SylvainCorlay for bqplot review.

@jasongrout
Copy link
Member

The bqplot example looks fine from a quick glance, but I haven't run it.

widgets.html Outdated
window.requirejs.onError = function(e) {
console.log(e);
// hack! Remove the anonymous require call from https://unpkg.com/@jupyter-widgets/[email protected]/dist/embed-amd.js
// window.require(['@jupyter-widgets/html-manager/dist/libembed-amd'], ...
Copy link
Member

Choose a reason for hiding this comment

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

These lines are not needed, right? Do we need to override the error function at all?

Copy link

@MariuszJurowicz MariuszJurowicz Oct 23, 2017

Choose a reason for hiding this comment

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

As described in comment we need to resolve the issue with anonymous require call from https://unpkg.com/@jupyter-widgets/[email protected]/dist/embed-amd.js.
Otherwise if you remove this onerror overridden method the requirejs will throw an error:

Error: Mismatched anonymous define() module: [object Object]
http://requirejs.org/docs/errors.html#mismatch
    at makeError (require.min.js:1)
    at v (require.min.js:1)
    at Object.o [as require] (require.min.js:1)
    at requirejs (require.min.js:1)
    at embed-amd.js:274986
    at Promise (<anonymous>)
    at requirePromise (embed-amd.js:274980)
    at Object.renderWidgets (embed-amd.js:275006)
    at embed-amd.js:281516

and stop working. So the examples won't work as their widgets won't be loaded by requirejs.

Copy link

@MariuszJurowicz MariuszJurowicz Oct 24, 2017

Choose a reason for hiding this comment

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

@jasongrout
After debugging the issue of Mismatched anonymous define() module it turned out that it's caused by the anonymous module inside the https://buttons.github.io/buttons.js. After adding the requirejs to http://jupyter.org/widgets the buttons.js script runs the

if typeof define is "function" and define.amd
  define [], { render: render }

which causes the error to be thrown. It has nothing to do with the ipywidgets v7.

The github-buttons issue: buttons/github-buttons#31

Copy link
Member

Choose a reason for hiding this comment

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

Huh. This is part of the reason why we created an html manager that did not use require - because introducing require on a page changes how the js on the page works.

Choose a reason for hiding this comment

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

But still, the example widgets are using the amd modules and depend on RequireJS.
So using the non amd version of ipywidgets will not resolve the issue here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afaik the simplest fix would be to not include buttons.js.
it's used to create the github and twitter buttons in the footer.
these buttons could be replaced with simple html.

Copy link
Member

@SylvainCorlay SylvainCorlay Oct 24, 2017

Choose a reason for hiding this comment

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

I would be 👍 on not including this js at all. cc @ellisonbg

Choose a reason for hiding this comment

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

I've got an answer from the lib author. His solution is to load the lib via the RequireJS. But this would force us to include the RequireJS on other pages as well. I'll provide the non js solution for the Follow button though.

@SylvainCorlay
Copy link
Member

I approve the PR, lacking context on the question on the overriding of onError.

@scottdraves
Copy link
Contributor Author

hack and buttons.js removed.

@SylvainCorlay
Copy link
Member

I am fine with merging as is. Waiting a bit to give @jasongrout a chance to comment.

@SylvainCorlay
Copy link
Member

@scottdraves btw, it seems that this needs a rebase on master.

@scottdraves
Copy link
Contributor Author

rebased

@SylvainCorlay SylvainCorlay merged commit ffe3885 into jupyter:master Oct 25, 2017
@scottdraves
Copy link
Contributor Author

thanks!

@SylvainCorlay
Copy link
Member

Merged! ✨ 🎉

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.

5 participants