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

Update for Jlab 3 #90

Merged
merged 17 commits into from
Mar 5, 2021
Merged

Update for Jlab 3 #90

merged 17 commits into from
Mar 5, 2021

Conversation

ianhi
Copy link
Contributor

@ianhi ianhi commented Feb 27, 2021

Building off of #83

I tested this locally and it worked in both classic notebook and jlab3.

Things I did not do:

  1. Update python packaging

Waiting until jupyter-widgets/widget-cookiecutter#87 resolves.

  1. Fix the tests

They were broken before and there is a draft fix in #86 so I didn't want to overcomplicate this PR.

@ianhi ianhi mentioned this pull request Feb 27, 2021
@ianhi
Copy link
Contributor Author

ianhi commented Feb 27, 2021

attn all those reviewing the previous PR: @martinRenou @jtpio @jasongrout @vidartf

@jtpio
Copy link
Member

jtpio commented Mar 1, 2021

Thanks @ianhi for picking this up!

Changes look good. Maybe it could be worth checking the state of #86 to see if it can already be merged.

@ianhi
Copy link
Contributor Author

ianhi commented Mar 2, 2021

Looks like the output of python -c "from notebook.nbextensions import validate_nbextension; import sys; sys.exit(validate_nbextension('jupyter_widget_testwidgets/extension') or 0)"

is getting mangled somehow into:

['        - require? \x1b[31m X\x1b[0m jupyter_widget_testwidgets/extension']

Which I'm also seeing locally. Is that a real problem with the widget or an issue with python and printing out color characters?

@ianhi
Copy link
Contributor Author

ianhi commented Mar 2, 2021

Oh hmm, there's clearly an X in there, but the enable command worked fine gives an OK
image

@ianhi
Copy link
Contributor Author

ianhi commented Mar 2, 2021

It turns out it is a real error. This is the second time I accidentally broke notebook while working on this PR.

@jtpio
Copy link
Member

jtpio commented Mar 2, 2021

Which I'm also seeing locally. Is that a real problem with the widget or an issue with python and printing out color characters?

Probably because of the coloring?

What does jupyter nbextension list give locally? Could it be an issue with the nbextension data files not being copied to the right location?

@ianhi
Copy link
Contributor Author

ianhi commented Mar 2, 2021

Looking into it more I think this is due to the changes to _nbextension_paths in __init__.py

I copied the version from the widget cookiecutter: https://github.com/jupyter-widgets/widget-cookiecutter/blob/969471848da747d5aa562b8ab00988e3117e4ccf/%7B%7Bcookiecutter.github_project_name%7D%7D/%7B%7Bcookiecutter.python_package_name%7D%7D/__init__.py#L25

which uses:

    return [{
        'section': 'notebook',
        'src': 'nbextension',
        'dest': '{{ cookiecutter.npm_package_name}}',
        'require': '{{ cookiecutter.npm_package_name }}/extension'
    }]

but originally this cookiecutter used the python path:

def _jupyter_nbextension_paths():
return [{
'section': 'notebook',
'src': 'nbextension/static',
'dest': '{{ cookiecutter.python_package_name }}',
'require': '{{ cookiecutter.python_package_name }}/extension'
}]

Is either of these more correct? I have no idea what notebook wants.

and since the npm package name has a slash in it @jupyter-widgets/jupyter-widget-testwidgets it's getting put into share/jupyter/nbextensions/@jupyter-widgets/jupyter-widget-testwidgets but it seems that notebook expects it to be in just .../nbextensions/jupyter-widget-testwidgets

Do notebooks for widgets work when there is a slash in the npm package name (or does the other cookiecutter work when there is a slash in the name)?

@ianhi
Copy link
Contributor Author

ianhi commented Mar 2, 2021

Ahh, this is discussed in the widget-cookiecutter: jupyter-widgets/widget-cookiecutter#80 I think the python name is the best approach

@jtpio
Copy link
Member

jtpio commented Mar 2, 2021

Ahh, this is discussed in the widget-cookiecutter: jupyter-widgets/widget-cookiecutter#80 I think the python name is the best approach

Looks like it would make sense for the nbextension yes.

@jtpio
Copy link
Member

jtpio commented Mar 3, 2021

@ianhi looks like CI is now passing after switching to python_name for the nbextension.

@ianhi
Copy link
Contributor Author

ianhi commented Mar 3, 2021

@ianhi looks like CI is now passing after switching to python_name for the nbextension.

Nice! thanks for fixing that. I've got to do lots of lab stuff today, but I'll try to test out a generate extension in both notebook and lab this evening just to make sure it really does show up properly.

@jtpio
Copy link
Member

jtpio commented Mar 3, 2021

I'll try to test out a generate extension in both notebook and lab this evening just to make sure it really does show up properly.

Thanks!
This is where some basic karma / jest tests help. We could also try to introduce end to end tests at some point too.

I just tried it locally with the following and it seems to be working fine 👍

cookiecutter https://github.com/ianhi/widget-ts-cookiecutter --checkout jlab-3
cd widgetexample
python -m pip install -e .

# check the extensions are installed
jupyter nbextension list
jupyter labextension list

# test in lab and notebook
jupyter lab example/introduction.ipynb
jupyter notebook example/introduction.ipynb

@ianhi
Copy link
Contributor Author

ianhi commented Mar 3, 2021

I just tried it locally with the following and it seems to be working fine +1

Awesome - I see the same!

So I think this is good to merge unless there are any complaints?

README.md Outdated Show resolved Hide resolved
Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit e07d534 into jupyter-widgets:master Mar 5, 2021
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.

3 participants