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

Fix issue where the script doesn't fire on page load or search. #4069

Merged
merged 2 commits into from
Aug 3, 2022
Merged

Fix issue where the script doesn't fire on page load or search. #4069

merged 2 commits into from
Aug 3, 2022

Conversation

mikesnoeren
Copy link
Collaborator

@mikesnoeren mikesnoeren commented Aug 3, 2022

The Problem/Issue/Bug:

This PR fixes part of this issue by triggering the fixTabs function on page load and on hash change.

Manual testing

Use this link to test, you'll notice the page automatically opening the Linux tab.

@rfay
Copy link
Member

rfay commented Aug 3, 2022

Thanks for working on this @mikesnoeren ! Please keep working on it!

If I go to https://ddev--4069.org.readthedocs.build/en/4069/users/install/docker-installation/#linux on a browser that's setting on the front page... It just lands on https://ddev--4069.org.readthedocs.build/en/4069/users/install/docker-installation, the base page, not on the tab.

However, I see that if I go to https://ddev--4069.org.readthedocs.build/en/4069/users/install/ddev-installation/#windows-wsl2 it does great AND it highlights the Windows WSL2 tab. So I think it's going to work, we just have some miles to walk yet.

Thanks!

@mikesnoeren
Copy link
Collaborator Author

If I go to https://ddev--4069.org.readthedocs.build/en/4069/users/install/docker-installation/#linux on a browser that's setting on the front page... It just lands on https://ddev--4069.org.readthedocs.build/en/4069/users/install/docker-installation, the base page, not on the tab.

I'm sorry but I can't reproduce that, everything works fine for me.
Could you give me the exact steps to reproduce the issue and the name and version of your browser?

@rfay
Copy link
Member

rfay commented Aug 3, 2022

Works for me in incognito also.

@rfay
Copy link
Member

rfay commented Aug 3, 2022

Test plan:

For each test, make sure to be on the base page, https://ddev--4069.org.readthedocs.build/en/4069

For each test, open a new tab.

@rfay
Copy link
Member

rfay commented Aug 3, 2022

@mikesnoeren I see somewhat unpredictable results. Most things work, but several of those failed. I guess I should look at the console for js errors? And when I return to them they often work. So we obviously have a huge step forward, but I'd appreciate if you could try those various things.

@rfay
Copy link
Member

rfay commented Aug 3, 2022

I just tried the first item (macOS, Chrome, incognito) a few times and it failed each time. Searched for steps, got the URL https://ddev--4069.org.readthedocs.build/en/4069/users/install/docker-installation/#linux, it landed on the macOS tab. Edit: Repeatable, nothing in browser console.

Edit: Just tried the same test (not incognito) in Firefox, same result, failed.

@mikesnoeren
Copy link
Collaborator Author

I disabled the navigation.instant feature for mkdocs-material which caused the page to act like a SPA.
This should fix the issues above as the docs now do an actual redirect instead of content injection when using search results..

If the navigation.instant feature is required by someone I can probably get it to work, but as all of my earlier fixes are temporarily this seemed like the best option for now.

@rfay
Copy link
Member

rfay commented Aug 3, 2022

Thanks, I'll give it a go!

@rfay
Copy link
Member

rfay commented Aug 3, 2022

Thanks so much, absolutely awesome!

@rfay rfay merged commit f935891 into ddev:master Aug 3, 2022
@rfay
Copy link
Member

rfay commented Aug 3, 2022

Thanks so much for solving this!

@mikesnoeren mikesnoeren deleted the improve-docs-hash-links branch August 11, 2022 12:06
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.

None yet

2 participants