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

✨ NEW: Option for download button #245

Merged
merged 7 commits into from
Nov 8, 2020
Merged

✨ NEW: Option for download button #245

merged 7 commits into from
Nov 8, 2020

Conversation

bknaepen
Copy link
Contributor

@bknaepen bknaepen commented Nov 1, 2020

This relates to the following open issue:

#244

I am completely new to this package but this proposal seems to be functional. I just duplicated lines I found for the issue button.

The tests are failing but I was unable to fix this.

@welcome
Copy link

welcome bot commented Nov 1, 2020

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! 🎉

@@ -17,6 +17,7 @@ navbar_footer_text =
extra_navbar = Theme by the <a href="https://ebp.jupyterbook.org">Executable Book Project</a>
extra_footer =
use_issues_button = False
use_download_button = False
Copy link
Member

@choldgraf choldgraf Nov 1, 2020

Choose a reason for hiding this comment

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

let's set this to True by default. I think that'll get the tests to pass. Though we should also add a test for this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That did it :-)

@@ -17,6 +17,7 @@ navbar_footer_text =
extra_navbar = Theme by the <a href="https://ebp.jupyterbook.org">Executable Book Project</a>
extra_footer =
use_issues_button = False
use_repository_button = False
use_download_button = False
Copy link
Member

Choose a reason for hiding this comment

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

hmm - use_repository_button should remain False (the default that we currently have), and use_download_button should be True (also our current default behavior)

Can you also add documentation for this feature to https://github.com/executablebooks/sphinx-book-theme/blob/master/docs/configure.md ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I obviously edited the wrong line 🤦🏻‍♂️. I hope I got it right this time...

I'll proceed to the documentation and be back.

@pradyunsg
Copy link
Member

Another The message was: Cell execution timed out.. Closing-reopening to re-trigger CI.

@pradyunsg pradyunsg closed this Nov 2, 2020
@pradyunsg pradyunsg reopened this Nov 2, 2020
@choldgraf choldgraf changed the title Option for download button ✨ NEW: Option for download button Nov 8, 2020
@choldgraf choldgraf merged commit a1fa391 into executablebooks:master Nov 8, 2020
@welcome
Copy link

welcome bot commented Nov 8, 2020

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

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

@choldgraf
Copy link
Member

Thanks very much for this! RTD timeout errors fixed so let's get it in!

@bknaepen
Copy link
Contributor Author

bknaepen commented Nov 9, 2020

@choldgraf I'm glad this feature was found useful, thanks for helping out :-). You mentioned above that a validation test would also be useful. I could work on this in the coming days when time permits but I need to dig a bit more on how to do this. What do you think?

@choldgraf
Copy link
Member

that would be great @bknaepen . I'd recommend checking out how the tests work for similar features: https://github.com/executablebooks/sphinx-book-theme/blob/master/tests/test_build.py#L217 and try to recreate those patterns. Wanna give that a shot?

@bknaepen
Copy link
Contributor Author

@choldgraf Yes I will try ;-)

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