-
Notifications
You must be signed in to change notification settings - Fork 50
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
🐛 FIX: Fix edge-case when page is '#' #132
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
CC: @pradyunsg @choldgraf If this is good with you, it would be great if we can get a release out of it :) |
hmmm this doesn't seem quite right - the clipboards are missing in the readthedocs build. Can you confirm this works on your machine? |
Ah, I just saw that the CI does a RTD build. Let me find out what's going on |
When building on Read the Docs, the parameter DOCUMENTATION_OPTIONS.URL_ROOT can return '#' and that returns an incorrect path to the svg icon for the copy button. Closes: executablebooks#121
@choldgraf I think this should be fixed by now |
cool - and this is probably obvious but just to confirm since we don't explicitly test for it here: this patch fixes your problem with the root doc not working properly? (can we think of a way to test that case? I don't think it's strictly necessary but would be good if it's easy) |
Yup, I tested it here: this is how it renders: https://cpython-devguide--713.org.readthedocs.build/#
I don't know what's the best way to test this. I have been checking the output of the RTD in the this repo CI when you click the root (as it adds the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although a non-blocking concern that we're doing one unnecessary round trip, that's similar to:
s = "string"
y = f"{s}"
An easy way to test this in this PR and make future breakage less likely, would be to add a code block to the documentation root for this too! |
@pradyunsg but there already is, isn't there? the landing page has code blocks in it. Am I missing something? |
Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>
Nope, I'm just a confused kiddo. :) |
Thanks a lot @choldgraf for the review and the quick release ❤️ |
When building on Read the Docs, the parameter
DOCUMENTATION_OPTIONS.URL_ROOT can return '#' and that returns an
incorrect path to the svg icon for the copy button.
Closes: #121