-
Notifications
You must be signed in to change notification settings - Fork 30
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
Furo: anchor links still broken for page table of contents #368
Milestone
Comments
Would be fixed by pradyunsg/furo#664. Fingers crossed that gets accepted 🤞 Otherwise, we will need to figure out how to vendor the JS code, between:
|
Eric-Arellano
added a commit
that referenced
this issue
Jun 20, 2023
Works around #368 while we wait to see what happens with the upstream PR. For now, it's too complex to vendor the JavaScript code, so we instead disable the problematic feature. Before: ![Screenshot 2023-06-20 at 3 11 58 PM](https://github.com/Qiskit/qiskit_sphinx_theme/assets/14852634/9647cc53-49af-4202-8a85-fbffba9712bc) After: ![Screenshot 2023-06-20 at 3 07 36 PM](https://github.com/Qiskit/qiskit_sphinx_theme/assets/14852634/9e8b3eaa-dc1c-4ed8-9c18-8d33290b0a5b)
Eric-Arellano
added a commit
that referenced
this issue
Jun 28, 2023
[sphinx-theme-builder](https://sphinx-theme-builder.readthedocs.io/en/latest/) allows us to use JavaScript/NPM in our Python packaging pipeline. Whereas right now we have to write raw CSS and JavaScript files, we can now pre-process those files. sphinx-theme-builder automates installing Node and NPM for users. It runs `npm run build` to pre-compile our code, which for now simply copies our CSS with `cp` to keep the code simple. sphinx-theme-builder gives some benefits: 1. Minify our CSS and JS, which is important for site loading performance. #283 2. No performance hit to splitting our CSS into multiple files 3. We can use Sass, which is more maintainable than CSS 4. We can more easily vendor and tweak Furo's JS code, which allows fixing #368 ## Changes folder structure Before: ``` src └── qiskit_sphinx_theme ├── ecosystem ├── furo # the core qiskit theme └── pytorch ``` After: ``` src └── qiskit_sphinx_theme ├── assets # Sass and JS for core qiskit theme ├── ecosystem ├── pytorch └── theme # the core qiskit theme ```
Eric-Arellano
added a commit
that referenced
this issue
Jul 3, 2023
Closes #368. I realized that we cannot fix this upstream. We always need to fork `furo.js` because the `3.5rem` for our qiskit-top-nav-bar needs to be hardcoded. It doesn't update Furo's `header.top` like I was hoping. Thanks to now using Webpack and `sphinx-theme-builder`, we can vendor the two JavaScript files without issue.
Eric-Arellano
added a commit
to Eric-Arellano/qiskit_sphinx_theme
that referenced
this issue
Jul 3, 2023
Closes Qiskit#368. I realized that we cannot fix this upstream. We always need to fork `furo.js` because the `3.5rem` for our qiskit-top-nav-bar needs to be hardcoded. It doesn't update Furo's `header.top` like I was hoping. Thanks to now using Webpack and `sphinx-theme-builder`, we can vendor the two JavaScript files without issue.
Eric-Arellano
added a commit
that referenced
this issue
Jul 3, 2023
Closes #368. I realized that we cannot fix this upstream. We always need to fork `furo.js` because the `3.5rem` for our qiskit-top-nav-bar needs to be hardcoded. It doesn't update Furo's `header.top` like I was hoping. Thanks to now using Webpack and `sphinx-theme-builder`, we can vendor the two JavaScript files without issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
#340 fixed us not correctly showing the element when going to an anchor link.
But the page table of contents is still wrong with showing what is actively selected:
Whereas if I scroll down the equivalent of our Qiskit top nav bar, it works:
We need to update our visual regression test to ensure this works. We should consider not using a screenshot and instead checking if the expected HTML element has the class for marking it as the currently selected page. That makes the test less sensitive to styling changes.
The text was updated successfully, but these errors were encountered: