Fix current page detection for clean URLs#2993
Conversation
qaqland
force-pushed
the
current-page-active
branch
2 times, most recently
from
bee3fca to
1404b89
Compare
Previously, the sidebar JavaScript logic for detecting the "active" page relied on URL and assumed it would end with .html. When using clean URLs without this suffix, this logic would fail to correctly highlight the current page in the sidebar. This change moves the current page identification to server-side rendering by adding a data-current-link attribute to the HTML root element. Fixes rust-lang#2962
qaqland
force-pushed
the
current-page-active
branch
from
1404b89 to
4d0d4e5
Compare
|
@ehuss , could you please review this when you have a moment? |
|
Related PR: #3038 |
It's latest mdbook 0.5.2, but with rust-lang/mdBook#2993. jonathanpallant/mdBook@7e285be
|
I tested this and it works great. See https://f74d75b4.ferrous-systems-rust-exercises.pages.dev/latest/book/nrf52/usb-exercise. |
| if (current_page.endsWith('/')) { | ||
| current_page += 'index.html'; | ||
| } | ||
| const current_link = document.documentElement.dataset.currentLink; |
There was a problem hiding this comment.
Unfortunately this would be a breaking change if the user has a custom index.hbs. I try to be a little cautious about making those kinds of changes.
| // Set a dummy path to ensure other paths (e.g. in the TOC) are generated correctly | ||
| data_404.insert("path".to_owned(), json!("404.md")); | ||
| data_404.insert("content".to_owned(), json!(html_content_404)); | ||
| data_404.insert("current_link".to_owned(), json!("404.html")); |
There was a problem hiding this comment.
Nothing to change here. Just noting that this isn't technically correct because the output filename can be customized. However, since there isn't anything in the sidebar for it to match, I don't think it matters.
| ctx.data.insert("title".to_owned(), json!(title)); | ||
| ctx.data | ||
| .insert("path_to_root".to_owned(), json!(fs::path_to_root(path))); | ||
| let current_link = path.with_extension("html").to_url_path(); |
There was a problem hiding this comment.
I'm noticing that current_link isn't being set in the print page.
I don't think it matters too much because there is no sidebar navigation for the print page.
Offhand I don't think it is required, but I do have a little unease about having a missing variable.
|
Thanks for the PR! I've decided to go with a simpler solution in #3028. I appreciate that this PR has a somewhat cleaner or more principled way of communicating the correct page to the TOC. However, I'm a little concerned with the additional complexity this adds. Hopefully the other change provides the functionality that you need. And if not, let me know. There may be future extensions like #3047 which would provide first-class support for clean urls, which would hopefully deal with redirects by avoiding them. |
|
☔ The latest upstream changes (possibly #3028) made this pull request unmergeable. Please resolve the merge conflicts. |
rustbot
added
the
S-waiting-on-author
|
Thank you for your review; happy to see that the problem can be resolved. |
5 participants
Previously, the sidebar JavaScript logic for detecting the "active" page relied on URL and assumed it would end with .html. When using clean URLs without this suffix, this logic would fail to correctly highlight the current page in the sidebar.
This change moves the current page identification to server-side rendering by adding a data-current-link attribute to the HTML root element.
Fixes #2962