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

Kernel Status #459

Merged
merged 17 commits into from
Sep 30, 2021
Merged

Conversation

stevejpurves
Copy link
Collaborator

@stevejpurves stevejpurves commented Aug 16, 2021

responding to issue #271 to make first-class support for a Status Widget.

Previously, users had to rely on functions in the examples folder in order to add status fields to their page. In circumstances where a generic status display will fit, this is now provided within thebelab.

The widget is minimally styled and styles can be easily overridden.

image

This could also be extended to help manage the multiple kernel case requested in #343 and to give more information back on the kernel type.

TODO:

  • add plain HTML example
  • add examples in the main docs
  • add some tests around KernelStatus
  • fix the readthedocs build
  • added a built in active button
  • updated examples to show activate button and launch button as separate examples. The launch button showing dynamic library loading
  • moved all library initialization into the DOMContentsLoaded event handler
  • added User Interface page to docs

@stevejpurves
Copy link
Collaborator Author

stevejpurves commented Sep 20, 2021

I feel like this is ready to come in but it will be a pain to test locally in terms of the docs build ahead of either #472 or #469 alternative coming in.

The purpose of this was to really, get this common functionality inside the library instead of relying on consumers implementing it from the examples.

I think this meets that and I have not gone as far as bringing in the sphinx-thebe button, not yet anyways. I think instead this gives a minimum easy activate buttons that works ok with the status widget.

@stevejpurves stevejpurves marked this pull request as ready for review September 20, 2021 21:05
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.

A few quick thoughts from me! This looks really nice, and I appreciate the documentation as well.

For trying this out, is the best way still to clone locally and set up a local dev environment?

src/options.js Outdated
@@ -27,8 +27,8 @@ const _defaultOptions = {
stripPrompts: false,
requestKernel: false,
predefinedOutput: true,
mountStatusWidget: true,
mountActivateWidget: true,
mountStatusWidget: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason not to make these true by default? If a page doesn't have the thebe-activate etc class, then this would just not have any effect right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it's a no-op when they are not on the page. 👍 I'll set them to true

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 looks good to me...I can't review the JS in depth but it seems reasonable. I'd be +1 on merging this in and seeing how it feels in practice and making iterative improvements from there. WDYT @stevejpurves ?

@stevejpurves
Copy link
Collaborator Author

planning on bringing this in tomorrow then - @moorepants @TimStewartJ your build doesn't have anything relying on the thebe-status or (new so unlikely) thebe-activate CSS classes?

@moorepants
Copy link
Collaborator

No, we're good. We also don't run the tip of master in our plugin, so we can check these things on new releases. This is our code that uses thebe, for reference: https://github.com/LibreTexts/ckeditor-binder-plugin

@choldgraf
Copy link
Collaborator

choldgraf commented Sep 28, 2021

Cool - as long as this doesn't disrupt @moorepants and his community then I'm +1 giving this a whirl!

@stevejpurves stevejpurves merged commit 3356dc0 into jupyter-book:master Sep 30, 2021
@choldgraf
Copy link
Collaborator

🚀

@stevejpurves stevejpurves deleted the feat/kernel-status branch November 18, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants