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

Update website theme for newer Sphinx #15365 #22872

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

lorsanta
Copy link
Contributor

@lorsanta lorsanta commented Nov 6, 2024

Ref #15365

I replaced the custom theme with the last version of sphinx_rtd_theme, and then added the changes found by diffing the emscripten custom theme with sphinx_rtd_theme version 0.1.6

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Wow! Thanks for working on this. This is awesome. I wonder if we could publish the output somewhere temporary so that is might be viewed before we land this and so we can compare the old and new output?

site/source/conf.py Outdated Show resolved Hide resolved
site/source/docs/api_reference/html5.h.rst Show resolved Hide resolved
site/source/docs/index.rst Outdated Show resolved Hide resolved
@sbc100
Copy link
Collaborator

sbc100 commented Nov 6, 2024

You will likely need to update all the sphinx-related stuff in requirements-dev.txt.

@lorsanta
Copy link
Contributor Author

lorsanta commented Nov 7, 2024

Wow! Thanks for working on this. This is awesome. I wonder if we could publish the output somewhere temporary so that is might be viewed before we land this and so we can compare the old and new output?

I guess I could create a github page If necessary

@sbc100
Copy link
Collaborator

sbc100 commented Nov 7, 2024

Wow! Thanks for working on this. This is awesome. I wonder if we could publish the output somewhere temporary so that is might be viewed before we land this and so we can compare the old and new output?

I guess I could create a github page If necessary

I wonder if we could setup CI such that the build-docs CI step automatically publishes somewhere temporary?

@kripken
Copy link
Member

kripken commented Nov 7, 2024

@sbc100 that would be ideal, but unless we have an idea for a quick way to do it, I think these updates are rare enough that it isn't worth waiting on such a feature. I'd be fine with building this PR locally to view the changes before merging, in particular.

@lorsanta Looks like there are some errors on build-docs?

@sbc100
Copy link
Collaborator

sbc100 commented Nov 7, 2024

@sbc100 that would be ideal, but unless we have an idea for a quick way to do it, I think these updates are rare enough that it isn't worth waiting on such a feature. I'd be fine with building this PR locally to view the changes before merging, in particular.

Yup, that sounds find to me. As long as the new site can be reviews somewhere before this PR lands. Perhaps you (@kripken) could publish the results under https://emscripten.org/docs/testing/?

@kripken
Copy link
Member

kripken commented Nov 7, 2024

@sbc100 I can upload it, sure. How about emscripten.org/testing/? (/docs/ is already part of the proper output, so it would mix them) Or maybe even /_testing/ to make it clear it is internal.

@lorsanta Building locally, I had to do

-docutils==0.22
+docutils==0.21.2

because it couldn't find 0.22.

Pages look valid after that (and also a little nicer/more modern!) but I also ran into a problem: doing a search (typing in the search box on the upper left, where it says "Search docs", and then pressing enter) hangs on "Searching...", and the JS console shows an error

searchtools.js:281 Uncaught ReferenceError: Stemmer is not defined
    at Object._parseQuery (searchtools.js:281:21)
    at Object.query (searchtools.js:416:91)
    at Object.setIndex (searchtools.js:226:14)
    at searchindex.js:1:8

@sbc100
Copy link
Collaborator

sbc100 commented Nov 7, 2024

@sbc100 I can upload it, sure. How about emscripten.org/testing/? (/docs/ is already part of the proper output, so it would mix them) Or maybe even /_testing/ to make it clear it is internal.

Either way SGTM.

@kripken
Copy link
Member

kripken commented Nov 7, 2024

Uploaded: https://emscripten.org/_testing/

@kripken
Copy link
Member

kripken commented Nov 7, 2024

Another difference I just noticed is that on the main page, under "About this site" the "Index" has vanished in the new version. Is that intentional?

@sbc100
Copy link
Collaborator

sbc100 commented Nov 7, 2024

Another difference I just noticed is that on the main page, under "About this site" the "Index" has vanished in the new version. Is that intentional?

The anchors seems a little off too. When I use the index on the left to click on things like https://emscripten.org/_testing/docs/tools_reference/settings_reference.html#maximum-memory it seems to scroll down the that place in the index, but not the MAXIMUM_MEMORY anchor on the main page on the right.

@lorsanta
Copy link
Contributor Author

lorsanta commented Nov 7, 2024

Pages look valid after that (and also a little nicer/more modern!) but I also ran into a problem: doing a search (typing in the search box on the upper left, where it says "Search docs", and then pressing enter) hangs on "Searching...", and the JS console shows an error

Sorry my bad, I forgot to update search.html. I'll commit the change later with everything else.

@lorsanta Looks like there are some errors on build-docs?

emscripten-ci has python-3.8.10 so I can't use sphinx==8.1.3, I guess I'll have to change requirements-dev.txt again

Another difference I just noticed is that on the main page, under "About this site" the "Index" has vanished in the new version. Is that intentional?

Oh no it isn't, probably I forgot to add something in layout.html.

The anchors seems a little off too. When I use the index on the left to click on things like https://emscripten.org/_testing/docs/tools_reference/settings_reference.html#maximum-memory it seems to scroll down the that place in the index, but not the MAXIMUM_MEMORY anchor on the main page on the right.

I'll look into these issues in the next few days.

@kripken
Copy link
Member

kripken commented Nov 7, 2024

Sounds good, thanks @lorsanta ! This is a really nice PR, will be great to upgrade this.

requirements-dev.txt Outdated Show resolved Hide resolved
requirements-dev.txt Outdated Show resolved Hide resolved
@lorsanta
Copy link
Contributor Author

The anchors seems a little off too. When I use the index on the left to click on things like https://emscripten.org/_testing/docs/tools_reference/settings_reference.html#maximum-memory it seems to scroll down the that place in the index, but not the MAXIMUM_MEMORY anchor on the main page on the right.

The issue was caused by t[0].scrollIntoView() in theme.js, so I commented it out


# Pin docutils because newer versions are not compatible with sphinx 2.4.4
docutils==0.17.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about if we simply remove all of these dependencies except for the sphinx one ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that sphinx 7.1.2 has jinja >= 3.0 as a requirement.

I think it's best to keep jinja2==3.0 to avoid future releases of jinja2 to break things.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its best to only include direct dependencies here, up until the point where we have some reason to do otherwise.

But i'm no expert so if you think this is safer/better then I'm fine with it.

How about the rest of these indirect deps, can we remove those?

@sbc100
Copy link
Collaborator

sbc100 commented Nov 18, 2024

I think you can fix the mypy failure by adding site/source/_themes/ to exclude in .mypy.ini

@sbc100
Copy link
Collaborator

sbc100 commented Nov 18, 2024

@kripken can you re-publish with the latest changes?

@kripken
Copy link
Member

kripken commented Nov 19, 2024

@sbc100 uploaded

@kripken
Copy link
Member

kripken commented Nov 19, 2024

Looks like "Index" is back in the contents, and search works (though the results are different than before - but I don't know if they are better or worse 😄 I tested with "asyncify" in both cases).

Overall things look good to me - I can't see anything wrong.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

3 participants