-
Notifications
You must be signed in to change notification settings - Fork 501
Update navigation logic to maintain page context #2801
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
base: master
Are you sure you want to change the base?
Conversation
|
Nice, thank you! It needs a bit more work (not all by you):
|
I'm on Linux Fedora 43, I tested using firefox. I could also test chrome, and I have a windows laptop I could test firefox and chrome on as well.
You mean as in a webpage? I could just make the github pages endpoint available on my fork. |
That would be super helpful! |
Should work now: https://langestefan.github.io/Documenter.jl To test: Redirect to home pageThe following page is not available on the Switching to https://langestefan.github.io/Documenter.jl/v1.16. Stay on page when switching versionLinks that are available on all versions, for example https://langestefan.github.io/Documenter.jl/dev/lib/public/ So switching to Still need to update the bannerThis banner that appears at the top still links to the home page and not the current page:
Direct links do not forward to the right pageA direct link to https://langestefan.github.io/Documenter.jl/v1.16/test-another-feature/ still yields a 404. So this only works if you use the version drop down menu. I'm personally okay with keeping it that way. |
It would be nice if this could be directly automated in deploydocs, which has access to the whole gh-pages branch. But we should make sure any link rewriting isn't going to mess with Vitepress. |
|
I'm not sure that belongs in this PR. I'd like to limit it to fixing the version button |
|
Very good. The JavaScript code looks plausible to me and is clearly documented. So let's try to wrap it up then?
Then I hope we can merge this soon. The "upgrade existing installations feature" can be implemented separately. |
|
@fingolfin The banner doesn't seem to be functioning correctly, no matter what forwarding logic I try it will not bring me back to the same page. After trying for a few hours, I conclude that I unfortunately don't have the JS skills to fix it (or even understand what's going on). I suggest we leave the banner what it is for now. Regarding prettier. I ran the prettier formatter locally and pushed the changes, but CI prettier check still fails. There is no debug output at all on the CI so there is nothing for me to go on. It would be really helpful for future devs if you could add maybe a pre-commit file or something so I can replicate what the CI is doing locally. If the CI can't tell me what's wrong then I can't fix it. |
|
I've not used prettier myself, all I know is what the Documenter manual says about it:
Well and also that https://prettier.io/docs/install has some instructions on using it, but that's for v3 and for some reason Documenter uses v2; I am sure someone who knows npm will easily be able to adapt those instructions to run v2 (and my guess is that ChatGPT and friends will also help doing that; but I am not going to go down that rabbit hole, I already have enough on my plate...) Regarding making this more convenient for contributors: I agree! See also issues #2528 and #2529 (in short: "someone" should indeed make this nicer, now that "someone" just needs to show up ... :-/ ) (BTW that someone may also have to look into forking |
I've been keeping this at v2 because the (official) Prettier VS Code extension is stuck on v2 😞 |
I think Biome would be a good replacement. It has both a github CI action and a pre-commit hook. That way you can tell devs to run pre-commit with your own provided config file, and it will produce identical results to the github CI. |
|
Seems Biome also has first party VS Code support. Sounds good. Would you be interested in working on that, @langestefan ? Or, alternatively, to open an issue suggesting this? |
|
It is somewhat unfortunate that the But the perfect shouldn't be enemy of the good: this PR is already a great improvement. Perhaps we can open an issue after merging it which requests that a similar change be implemented in (Likewise an issue could be opened for adding tooling which updates the JS of older documentation versions so that they also benefit from this change). |
Sure! I will open an issue first to discuss.
I will take another look at it again this weekend. If I can fix it I will make a PR. |
|
I figured out that I can run prettier v2 via I've pushed an update to this PR. |
|
Hm, weird, prettier check still fails. Locally I see this: |
Context: #2799
Tested locally, seems to work correctly.
/stable/page.htmlto/dev/page.html./dev) when/dev/page.htmlis not available.