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: display external links in toctree #1055
Conversation
Cheers @12rambau looks good, maybe add in a test that covers the external url case? |
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.
This looks good to me! A few quick comments but they don't need to be blocking IMO:
- Could we test this out in our documentation so that we know how it looks re: CSS? Maybe in the
examples
section somewhere? - Given that this builds on @chrisjsewell's implementation and supercedes that PR, I think he should get attribution for the change. I know it's a bit tricky to do this via GitHub's UI but I just want to note this. Maybe we can cross-link his PR in the commit message and give him thanks
You can push a (possibly empty) commit with "Co-authored by Chris Sewell email@addr.ess" in the extended commit message. |
@drammock ah right - I think then we can use a "Squash commit" and give Chris attribution in the message there. That should be doable by the UI |
if he shows up in a "Co-authored by" line in one of the un-squashed commits, we don't need to write anything additional ourselves: GitHub will add the co-author lines to the squash commit message automatically. You're right that we could skip the empty-commit step and manually add the co-authorship line to the commit message when squash-merging... but I usually don't do it that way because it forces you to remember to do it at exactly the right time, only through the web interface, and it's annoying to fix it if you forgot. Pushing an empty commit can be done whenever. |
I thought @chrisjsewell PR was an example of what can be done but i'm completely ok to give him credit in this PR. Please open an empty PR on my branch and I'll accept it |
I'm happy to just make a merge commit when we merge this in. IMO let's focus on getting this PR to a state ready to merge. I think the only thing remaining is to have an example in the docs to see how this looks |
@choldgraf, I just moved the changelog from the external link list to a normal toctree element |
@12rambau looks like at least on mobile, the "external" icon is not there when external links are added to the TOC (this is what I was worried about). Can you add that in and then I think we are good to go |
It works from my side as well, I assume the .css file was stuck in my browser cache.I think we are good to go. @choldgraf is it ok for you ? |
Ah yep - it looks good to me as well now! I merged and gave attribution to @chrisjsewell in the commit message |
Cheers all! |
sorry to be a party-pooper, but the co-author attribution didn't work (i.e., it won't show up as co-authored by Chris S. in the GitHub UI). The syntax is very particular; it's documented here. It has to be like this:
instead it was
I can force-push a fix to |
Oops, didn't realize that this was super specific. I'm also happy to just provide extra attribution via the changelog update or something 🤷 . It feels like a bad idea to force-push to main :-/ |
🎶 force push, force push 🎶 (up to you guys though) |
hmm, if it were HEAD I would do it in a heartbeat, but now there's an RC on top of it... changing the commit hash of the RC commit will cause some headaches... plus the more I think about it, I can't in good conscience make everyone with a currently up-to-date clone have to delete and re-create their local |
Fix #913
Superced #1054
@chrisjsewell, I itterated from your suggestion and used
urlib
to decide wether or notpage
is an absolute url.I'm adding a class to the elements as well:
nav-internal
andnav-external
to follow what is done with the elements added from the conf.py filepydata-sphinx-theme/src/pydata_sphinx_theme/__init__.py
Line 345 in 7119c24
Let me know what you think