chore: Fix `navrel`/`tocrel` URL resolve behaviors when using `Site Relative URL`

dotnet / docfx

Static site generator for .NET API documentation.

https://dotnet.github.io/docfx/

MIT License

3.94k stars 839 forks source link

chore: Fix `navrel`/`tocrel` URL resolve behaviors when using `Site Relative URL` #10032

Closed filzrev closed 6 days ago

filzrev commented 1 week ago

This PR intended to fix some problem that discussed at #10028

On current docfx modern template implementation. navrel / tocrel absolute URL is resolve by using following code.

`new URL('toc.json', window.location.href)

So when I've modified docfx generated pages URLs to Site Relative URL (That starts with '/') based URL. JavaScript errors are occurred on non-site-root pages.

This PR fix this issue by adding dedicated code path for Site Relative URL.

Test I've manually confirmed following patterns and it works as expected.

Default page (That use relative URLs)
Rewriting HTML page URLs to Site Relative URL .
Rewriting HTML page URLs to Absolute URL.

And it also tested on default template environment. It ssems default template processing navrel / tocrel URL as plain string. so no modification needed.

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.84%. Comparing base (fe673ec) to head (a413786). Report is 215 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #10032 +/- ## ========================================== + Coverage 74.31% 78.84% +4.52% ========================================== Files 536 540 +4 Lines 23189 23479 +290 Branches 4056 4066 +10 ========================================== + Hits 17234 18513 +1279 + Misses 4853 3824 -1029 - Partials 1102 1142 +40 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rmannibucau commented 1 week ago

Not sure it can fix #10028 since #10028 is really about loading assets, so fixing only the js will make it a bit less broken but still broken. Guess a more transversal and not related to a particular theme solution is more desired, no?

filzrev commented 6 days ago

Close this issue.

Because I've changed approach on my code not to use <base> tags. (Instead rewrite entire HTML URLs for 404.md to Site Relative URL on site deployment timing)

Workflow: https://github.com/filzrev/docfx_sample_404_notfound/blob/main/.github/workflows/build_docs.yml#L91-L97
Script to rewrite URLs: https://github.com/filzrev/docfx_sample_404_notfound/blob/main/.github/scripts/Rewrite404Page.csx

This PR contents might be useful for people who want to use <base> tag on custom docfx layout page. Thought currently there are not such users at this time. So close this PR.

rmannibucau commented 6 days ago

Side note/warning: this is limited to default template usage AFAIK. It will not handle all the links in meta (rdf and friends) and data-xxx attributes. So customizations to the csx can be needed. (mainly for future readers, not asking any actions)