Skip to content
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

Scroll navigation to show the link to the current page #639

Closed
wants to merge 4 commits into from

Conversation

pdmosses
Copy link
Contributor

@pdmosses pdmosses commented May 4, 2021

After loading the page, find the link to it in the site nav, then scroll if needed.

Fixes #570.

After loading the page, find the link to it in the site nav, then scroll if needed.

Fixes just-the-docs#570.
@pdmosses
Copy link
Contributor Author

pdmosses commented May 4, 2021

I have no previous experience of writing JS, but this appears to work. Suggestions for improvement are welcome!

Not working on links with expanders
@pdmosses pdmosses marked this pull request as draft May 4, 2021 09:22
Support the following:
- The page location and/or the hrefs in the nav might have a trailing slash.
- The landing page isn't in the site-nav element.
All nav links in the JtD site now remain visible when clicked, as intended.
@pdmosses
Copy link
Contributor Author

pdmosses commented May 4, 2021

Screenshot after step 4 in #570:

Screenshot

All nav links in the JtD site now remain visible when clicked, as intended, without any JS errors.

@pdmosses pdmosses marked this pull request as ready for review May 4, 2021 13:44
pdmosses added a commit to pdmosses/just-the-docs that referenced this pull request May 4, 2021
pdmosses added a commit to pdmosses/just-the-docs that referenced this pull request May 4, 2021
@pdmosses pdmosses mentioned this pull request May 4, 2021
@adfoster-r7
Copy link

adfoster-r7 commented Apr 22, 2022

I have a docs site with a lot of navigation items as well; I made an extra tweak to this pull request on a fork to preference the user's previous scroll position over the calculated position if the menu item is still visible to the user: rapid7#1

I'd be happy to send that work over as a new pull request for your consideration, either to your branch, or the combination branch, or I can wait until the 0.4.0 release is confirmed 👍

@pdmosses
Copy link
Contributor Author

@adfoster-r7 many thanks for the improvements proposed in rapid7#1.

I'd be happy to send that work over as a new pull request for your consideration, either to your branch, or the combination branch, or I can wait until the 0.4.0 release is confirmed 👍

@mattxwang is currently considering whether the combination branch could be merged into the forthcoming v0.4.0 release, so I should avoid updating it at present. However, I think it would be good if you submit a PR to the combination branch. The nav-scroll branch used for this PR is not itself under review, and it might subsequently get closed in favour of the combination PR, so it seems best to avoid PRs on it.

In any case, I suppose I can test your proposed improvements by using your PR branch as a remote theme. But I don't know enough about the use of JS in Just the Docs to review your PR code myself.

@mattxwang mattxwang changed the base branch from main to v0.4.0 July 20, 2022 05:31
@mattxwang
Copy link
Member

Missed this on my first round, but I believe this was merged in to the RC by way of #578. @pdmosses feel free to reopen if I've made a mistake!

p.s. I accidentally made a mistake in updating this branch by duplicating the bundler line in the gemspec - if you do need to reopen, feel free to quickly update that line!

@mattxwang mattxwang closed this Jul 20, 2022
@pdmosses
Copy link
Contributor Author

@mattxwang yes, this was indeed included in #578, see my comment there.

@adfoster-r7 many thanks for the improvements proposed in rapid7#1.

I'd be happy to send that work over as a new pull request for your consideration, either to your branch, or the combination branch, or I can wait until the 0.4.0 release is confirmed 👍

@adfoster-r7 the combination branch has been merged in RC v0.4.0. If you could submit at PR based on that branch, we might be able to get in reviewed and merged before the release.

@mattxwang mattxwang mentioned this pull request Jul 28, 2022
@pdmosses pdmosses deleted the nav-scroll branch November 3, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation links are not always visible after clicking on them
3 participants