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

docs: Don't hide the version selector when scrolling down #5114

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

neongreen
Copy link
Contributor

by popular request (@arxanas)

NB: this also means the header will always say "Jujutsu docs", but maybe it's good because the user will be reminded that they are in the presence of mighty Jujutsu and will behave accordingly

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@neongreen neongreen requested a review from arxanas December 16, 2024 22:51
@arxanas
Copy link
Contributor

arxanas commented Dec 16, 2024

How do I test this? I can't get the version selector to appear locally (with or without this change).

@neongreen
Copy link
Contributor Author

Honestly idk

I just set up an override for versions.json in devtools to trick Mkdocs. There's probably a better way with mike, but I don't want to go as far as setting up a github fork etc etc just to test this.

Screen.Recording.2024-12-17.at.00.45.30.mp4

@arxanas
Copy link
Contributor

arxanas commented Dec 19, 2024

Thanks for the tip, I was able to override the versions.json file and test it out. For me, the only issue is that the default experience on narrow viewports will cut off the header text:

Screen Shot 2024-12-19 at 13 39 42

Normally, as you scroll down the page, the version selector will disappear, so the page header will usually be fully readable.

I think the ideal solution is to use a much smaller version selector dropdown icon for narrow viewports, but I would personally be fine to land this solution as is, even though it might be ugly. I'll ask in Discord if there are any other comments/concerns.

[aside] I was also mistaken here: I thought/recalled that the header would change dynamically depending on the section you're reading, but I can't reproduce that specifically, just that it transitions from "Jujutsu docs" to the normal page title. So it's even less concerning than I was originally thinking, since you're losing less contextual information than I was worried about. And, of course, the header is available normally inline in the text.

@@ -0,0 +1,22 @@
document$.subscribe(function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I tried this out and it seems to not work when I reload the page and keep the same scroll location. The header doesn't disappear until I scroll up and scroll down again.

However, removing the document$.subscribe and calling the function immediately seems to resolve the issue. My guess is that if the page is already scrolled down, then we never observe a mutation in the header for whatever reason. It looks like extra.js is embedded in the page after the target header element, so there should be a guarantee that the element can be addressed and we can install the mutation observer safely?

nit: Should this function also be an arrow function instead, like the other ones in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks, I’ll try without subscribe then

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.

2 participants