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

make the run, restart and restartall buttons optional #502

Merged
merged 3 commits into from
Oct 27, 2021

Conversation

twyair
Copy link
Contributor

@twyair twyair commented Oct 22, 2021

i made the buttons that appear in every cell optional by adding the following boolean options to the config button_run, button_restart, button_restartall

the reason i need this is that i embed the cells in revealjs slides and these buttons just get in the way of actual content

@welcome
Copy link

welcome bot commented Oct 22, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

src/options.js Outdated Show resolved Hide resolved
@stevejpurves
Copy link
Collaborator

stevejpurves commented Oct 25, 2021

@twyair thanks for the PR 👏 . I've just left a comment so this better aligns with other options.

It would also be good to mention this in the documentation - e.g. by adding a note in docs/ui.rst if you are comfortable doing that?

@twyair
Copy link
Contributor Author

twyair commented Oct 25, 2021

@twyair thanks for the PR clap . I've just left a comment so this better aligns with other options.

It would also be good to mention this in the documentation - e.g. by adding a note in docs/ui.rst if you are comfortable doing that?

done. though I'm not sure if what I wrote is clear enough or worded well.

@stevejpurves stevejpurves merged commit 9f3cbed into jupyter-book:master Oct 27, 2021
@welcome
Copy link

welcome bot commented Oct 27, 2021

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@choldgraf
Copy link
Collaborator

this looks great - thanks for the addition @twyair

one quick question to see what you all think: what about embedding these configuration options in a parent key like "cellButtons", so the structure would be:

...
cellButtons: {
  mountRunButton: true,
  mountRestartButton: true,
  mountRestartallButton: true,
}
...

it feels a bit more clearly-scoped that way, since I could imagine a situation where other Thebe buttons might exist on the screen as well.

but - tell me if I am over-engineering configuration here :-) I don't feel strongly about it

@stevejpurves
Copy link
Collaborator

The configuration is quite large and flat @choldgraf so i like this idea. Should we go further and group all "ui" related options?

...
ui: { // or 'controls', or?
    mountStatusWidget: true,
    mountActivateButton: true,
    mountRunButton: true,
    mountRestartButton: true,
    mountRestartallButton: true
}
...

or perhaps:

...
mount: { 
    statusWidget: true,
    activateButton: true,
    runButton: true,
    restartButton: true,
    restartallButton: true
}
...

@choldgraf
Copy link
Collaborator

I'd be fine with a UI sub-field (or maybe "cellUI"?), Whatever is the right key name that provides an intuition for what is inside. (Though we should do a depreciation cycle if anything will move that has been released)

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