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

[MNT] library global object rename #508

Merged
merged 10 commits into from
Nov 19, 2021

Conversation

stevejpurves
Copy link
Collaborator

@stevejpurves stevejpurves commented Oct 25, 2021

Towards #230

Summary of Changes

  • additional css selectors have been added as thebe- one for each instance of thebelab- selectors
  • window.thebelab is a reference to window.thebe
  • code in examples and test files .spec.js have been changes to use thebe objects/classes exclusively
  • whitespace fixes to some recently touched files
  • Additional Housekeeping was carried out in the activate widget docs
    • updated minimal_example.rst this to use the latest built in controls
    • moved the custom activate button to it's own example file

TODO:

  • document the change and deprecation cycle
  • additional testing

@choldgraf
Copy link
Collaborator

@stevejpurves could you give an idea of the major changes that have been made? I was expecting a pure "find/replace", but it seemed like there was also some content that had been added (e.g., the custom activate button). I think for things like "replace thebelab with thebe", it'd be best to just do a single thing in the PR so it's not confusing about what has changed.

Also - perhaps we can allow thebelab to work as well for another deprecation cycle or two? (e.g., make thebelab an alias for thebe?)

docs/examples/custom_activate_button.rst Show resolved Hide resolved
src/index.css Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/render.js Show resolved Hide resolved
src/render.js Show resolved Hide resolved
@stevejpurves
Copy link
Collaborator Author

stevejpurves commented Nov 1, 2021

@choldgraf thanks for the feedback. I added some comments and details in the PR description including a new TODO

  • Sorry there are some content changes in here that could have been done as a PR eslewhere (there are more todo) I made these as I was touching that example. (Basically I wanted to preserve what I was taking out of minimal_example.rst in a new example, and further changes are needed to make the docs consistent but I held off a full pass) I'll unsure more atomic PRs going forward.
  • I was planning this to allow for deprecation cycles - as css classes and objects are aliased when they are created - which should do it? I have made some inline comments to make it reasoning around the changes clearer and will do some more testing.
  • internal references to the thebe/thebelab objects and all examples have been updated to point people to using thebe

@stevejpurves
Copy link
Collaborator Author

Did some additional local testing and confirmed that setting bootstrap: false in options, and then manually calling thebelab.bootstrap() starts the library successfully and all cells are hooked up to kernels and can be run

@stevejpurves
Copy link
Collaborator Author

planning on bringing this one in
cc @choldgraf

@choldgraf
Copy link
Collaborator

Was just gonna ask what needs to happen to move it through 🙂 anything you need help with?

@stevejpurves stevejpurves deleted the mnt/complete-migration branch November 18, 2021 14:44
@choldgraf
Copy link
Collaborator

@stevejpurves did you implement this elsewhere or something?

@stevejpurves stevejpurves restored the mnt/complete-migration branch November 18, 2021 15:10
@stevejpurves stevejpurves reopened this Nov 18, 2021
@stevejpurves
Copy link
Collaborator Author

stevejpurves commented Nov 18, 2021

over zealous clean up on my part @choldgraf
Restored. I am just getting in other smaller changes before bringing this in.
Will rebase this shortly.

Copy link
Collaborator

@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.

This all looks good to me - I guess the main thing is to make sure we haven't missed a thebelab* somewhere in there, but it seems good to me!

I added a couple of non-blocking comments, thanks for this helpful maintenance step!

@@ -1,5 +1,12 @@
# Changelog

## 0.8.3 - To be confirmed
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this makes sense since thebelab still works - we should do a minor version bump when thebelab is removed though

docs/examples/custom_activate_button.rst Outdated Show resolved Hide resolved
docs/examples/custom_activate_button.rst Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@stevejpurves stevejpurves merged commit 15930c1 into jupyter-book:master Nov 19, 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.

2 participants