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

JupyterLab 1.x support #26

Merged
merged 1 commit into from
Sep 24, 2019
Merged

Conversation

kdunn926
Copy link
Contributor

@kdunn926 kdunn926 commented Sep 13, 2019

Great project you all have here.

I noticed #17 as well when I went to play around with this again and figured I'd take a shot at fixing it. These changes seem to work for me after following the steps in the labextension/vpython/README.md, I'm definitely not up to speed on the contribution guidelines for this project but please let me know if there are other things I need to do as part of this PR.

Cheers,
Kyle

@jcoady
Copy link
Contributor

jcoady commented Sep 14, 2019 via email

Copy link
Contributor

@mwcraig mwcraig left a comment

Choose a reason for hiding this comment

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

@kdunn926 -- thanks for opening this PR! Most of the demo notebooks I tried worked without a problem (and the one that didn't likely has nothing to do with your changes).

Could you please add your name to https://github.com/vpython/vpython-jupyter/blob/master/CONTRIBUTORS.md?

@jcoady and @BruceSherwood -- I'm ok with merging this if you are.

@@ -22,6 +22,7 @@ For a development install (requires npm version 4 or later), do the following in
npm install
npm install --save @jupyterlab/notebook @jupyterlab/application @jupyterlab/apputils @jupyterlab/docregistry @phosphor/disposable script-loader
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
npm install --save @jupyterlab/notebook @jupyterlab/application @jupyterlab/apputils @jupyterlab/docregistry @phosphor/disposable script-loader

I was able to omit this and still build the extension successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorporated this change in squash-commit f363918.

@@ -22,6 +22,7 @@ For a development install (requires npm version 4 or later), do the following in
npm install
npm install --save @jupyterlab/notebook @jupyterlab/application @jupyterlab/apputils @jupyterlab/docregistry @phosphor/disposable script-loader
npm run build
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
npm run build

I think this can be left out too (npm run build seems to happen as part of npm install)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorporated this change in squash-commit f363918.

"script-loader": "^0.7.2",
"typescript": "~2.9.2"
"typescript": "^3.6.3",
"file-loader": "^4.2.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"file-loader": "^4.2.0"
"file-loader": "^4.2.0",
"webpack": "^4.0.0"

Adding webpack eliminated this warning: "npm WARN file-loader@4.2.0 requires a peer of webpack@^4.0.0 but none is installed. You must install peer dependencies yourself."

Copy link
Member

Choose a reason for hiding this comment

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

Fine with me, Matt, but this is way outside my field of competence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Matt , I am OK with you merging this fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorporated this change in squash-commit f363918.

* Added some workarounds to ensure fonts are included in the webpack packing
* Simplify README steps
* Update contributors list
@mwcraig
Copy link
Contributor

mwcraig commented Sep 24, 2019

closing/reopening to try to trigger travis build...

@mwcraig mwcraig closed this Sep 24, 2019
@mwcraig mwcraig reopened this Sep 24, 2019
@mwcraig
Copy link
Contributor

mwcraig commented Sep 24, 2019

Thanks, @kdunn926, we'll try to get the release out this week!

@mwcraig mwcraig merged commit 0b0c153 into vpython:master Sep 24, 2019
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.

None yet

4 participants