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

Furo: anchor links still broken for page table of contents #368

Closed
Eric-Arellano opened this issue Jun 7, 2023 · 1 comment · Fixed by #455
Closed

Furo: anchor links still broken for page table of contents #368

Eric-Arellano opened this issue Jun 7, 2023 · 1 comment · Fixed by #455
Assignees
Milestone

Comments

@Eric-Arellano
Copy link
Collaborator

#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:

Screenshot 2023-06-07 at 11 51 16 AM

Whereas if I scroll down the equivalent of our Qiskit top nav bar, it works:

Screenshot 2023-06-07 at 11 51 38 AM

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.

@Eric-Arellano Eric-Arellano added this to the Furo milestone Jun 7, 2023
@Eric-Arellano Eric-Arellano self-assigned this Jun 8, 2023
@Eric-Arellano
Copy link
Collaborator Author

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:

  1. Set up webpack in our repo. Which is more complex than it seems. So that we don't break "sdists" (source distributions), we have to make sure that our Python's setup.py knows how to build webpack.
  2. Vendor the final webpack output. But it's not very human-understandable.
  3. Vendor the raw JavaScript files. They don't have things like modification and rewriting for compatibility, so it makes for a little bit worse performance and less compatibility with old browsers.

@Eric-Arellano Eric-Arellano removed this from the Furo 1.0 milestone Jun 20, 2023
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 Eric-Arellano added this to the Furo 1.0 milestone Jun 27, 2023
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 Eric-Arellano removed this from the Furo 1.0 milestone Jun 29, 2023
@Eric-Arellano Eric-Arellano added this to the Furo 1.0 milestone Jul 3, 2023
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
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant