executablebooks / sphinx-book-theme

A clean book theme for scientific explanations and documentation with Sphinx
https://sphinx-book-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
431 stars 197 forks source link

Add the possibility to disable launch buttons from Jupyter Book #692

Closed paugier closed 1 year ago

paugier commented 1 year ago

Fixes https://github.com/executablebooks/jupyter-book/issues/1463

welcome[bot] commented 1 year ago

Thanks for submitting your first pull request! You are awesome! :hugs:
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! :tada:

choldgraf commented 1 year ago

This seems like a reasonable fix to me, but I wanted to throw out one other idea because it might be cleaner:

The goal is for launch buttons only to show up if a user has configured them to do so. I think the real problem is these things:

So a way to fix this without changing config values would be to make launch buttons only show if at least one service is explicitly specified, rather than defaulting to mybinder.org. it would mean that users explicitly have to put mybinder.org, but maybe that's ok?

So i guess the decision point is: is it better to ask people to manually disable launch buttons if they don't want them, or better to manually enable mybinder.org rather than default to it

What do you think?

paugier commented 1 year ago

I don't have strong opinion on what is better ("manually disable" or "manually enable").

An advantage of the solution "manually disable" is that it is not a breaking change. Most users won't be affected by this change.

choldgraf commented 1 year ago

Now that I think about it more, I think we want the default in JB to be "no launch buttons unless you explicitly define a service". That's because otherwise there is no guarantee that the repository is actually buildable on mybinder.org . So we don't want to show a button to use binder if we don't know binder is even an option

paugier commented 1 year ago

Yes, it makes sense. Breaking change but for a good reason.

For people working with something else than Github it's even more convenient because the warnings will disappear without doing anything :slightly_smiling_face:

So if I understand correctly I guess this PR can be closed and replaced by a new PR that could also address #462.

choldgraf commented 1 year ago

closing this since it was superceded by #693 and the change to jupyter book