FIX: display external links in toctree

[12rambau]

Fix #913
Superced #1054

@chrisjsewell, I itterated from your suggestion and used urlib to decide wether or not page is an absolute url.
I'm adding a class to the elements as well:
nav-internal and nav-external to follow what is done with the elements added from the conf.py file

pydata-sphinx-theme/src/pydata_sphinx_theme/__init__.py

Line 345 in 7119c24

# Add external links defined in configuration as sibling list items

Let me know what you think

[chrisjsewell]

Cheers @12rambau looks good, maybe add in a test that covers the external url case?

[choldgraf]

This looks good to me! A few quick comments but they don't need to be blocking IMO:

1. Could we test this out in our documentation so that we know how it looks re: CSS? Maybe in the examples section somewhere?
2. 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

[drammock]

I think he should get attribution for the change.

You can push a (possibly empty) commit with "Co-authored by Chris Sewell email@addr.ess" in the extended commit message.

[choldgraf]

@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

[drammock]

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.

[12rambau]

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

[choldgraf]

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

[12rambau]

@choldgraf, I just moved the changelog from the external link list to a normal toctree element

[choldgraf]

@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

[12rambau]

This is very mysterious, my last change is supposed to add the external icon to all .nav-link.nav-external but it does not appear in the RDT build. On my local doc on the other hand it work as expected:

[drammock]

It looks correct to me?

narrow window:

[12rambau]

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 ?

[choldgraf]

Ah yep - it looks good to me as well now! I merged and gave attribution to @chrisjsewell in the commit message

[chrisjsewell]

Cheers all!

[drammock]

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:

fixes #913

Co-authored-by: Chris Sewell <chrisj_sewell@hotmail.com>

instead it was

co-authored with Chris Sewell (@chrisjsewell, chrisj_sewell@hotmail.com)

fixes #913

I can force-push a fix to main if people think it's important enough. Probably your call @chrisjsewell

[choldgraf]

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 :-/

[chrisjsewell]

🎶 force push, force push 🎶

(up to you guys though)

[drammock]

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 main branches.