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

Issue #12290 - Docs using Furo Theme W/ Dark Mode #12326

Merged
merged 11 commits into from
May 21, 2024

Conversation

samjirovec
Copy link
Contributor

Light Mode:
Screenshot 2024-05-14 at 7 04 42 PM

Dark Mode:
Screenshot 2024-05-14 at 7 04 37 PM

@bluetech
Copy link
Member

Quick link to preview: https://pytest--12326.org.readthedocs.build/en/12326/contents.html

Looks pretty nice to me. I noticed that in the API reference furo shows a warning about the table of contents: https://pytest--12326.org.readthedocs.build/en/12326/reference/reference.html

@samjirovec
Copy link
Contributor Author

samjirovec commented May 15, 2024

Ah, good find. When I get the chance today, I can go in a fix that.

Edit: cleaned up that error + one more page that had the same problem 🙂

@bluetech
Copy link
Member

To me this looks good.

Can you compare this PR with #9788 from @pradyunsg, in case that PR had something that's worth bringing over to this PR?

@@ -9,3 +9,4 @@ sphinxcontrib-svg2pdfconverter
# is the version that is assigned to the docs.
# See https://github.com/pytest-dev/pytest/pull/10578#issuecomment-1348249045.
packaging <22
furo
Copy link
Member

Choose a reason for hiding this comment

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

Should also remove the pallets-sphinx-themes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely. Will remove this. + removing some extras based on #9788

@The-Compiler
Copy link
Member

I love this in general! Not too happy about the "next trainings and events" getting a bit lost now, though:

image

image

Is there a nice way we can inject some custom CSS or so to fix that?

@nicoddemus
Copy link
Member

nicoddemus commented May 16, 2024

Overall I like the new theme!

The code highlighting for the light mode looks a bit off to me, but the highlighting for dark mode looks great. Perhaps that's something we can adjust later.

Is there a nice way we can inject some custom CSS or so to fix that?

Perhaps a small site-wide announcement? https://pradyunsg.me/furo/customisation/#announcement

I do not mean putting the entire information in the announcement, perhaps the announcement could be just a one liner "Next training/sprint: June 11th" with a link to a page with full details?

@samjirovec
Copy link
Contributor Author

I love this in general! Not too happy about the "next trainings and events" getting a bit lost now, though:

...

Is there a nice way we can inject some custom CSS or so to fix that?

definitely a good callout. Im pretty new to rst, so I'll see if we can get it to be more prominent. The solution may be something like that announcement banner referenced above: #12326 (comment)

@samjirovec
Copy link
Contributor Author

@nicoddemus @The-Compiler Did some experimenting with using that announcement bar but was having some problems with that static url. I reverted the sidebar for the trainings, here's a quick example of how that looks:
Screenshot 2024-05-16 at 9 24 36 PM
Screenshot 2024-05-16 at 9 24 41 PM

@pradyunsg
Copy link

I'd suggest also unsetting the pygments theme (I think it's called sphinx) in conf.py as well.

@@ -9,3 +8,4 @@ sphinxcontrib-svg2pdfconverter
# is the version that is assigned to the docs.
# See https://github.com/pytest-dev/pytest/pull/10578#issuecomment-1348249045.
packaging <22

Choose a reason for hiding this comment

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

You should be able to unpin this too since this was pinned due to the existing theme: #10578 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unpinned it + removed sphinx theme. This will affected only the light mode code snippets.

@nicoddemus
Copy link
Member

I'd suggest also unsetting the pygments theme (I think it's called sphinx) in conf.py as well.

Ahh the syntax highlight for the light theme is now much better. ❤️

Thanks!

I'm approving, pending @The-Compiler's opinion on how the training looks (however this is of course something we can tweak/improve later).

@ferdnyc
Copy link

ferdnyc commented May 20, 2024

Sidebars

One difference with the new theme: The layout breakpoints are all expressed in em units now (the old theme used px), and in practice they're a lot bigger.

For example, with the old theme (on an unscaled desktop display, with the browser windowed), the left-hand sidebar only collapses itself when the window is less than 767px wide.

With this theme, the sidebar doesn't open until the window is more than 67em wide, which in practice ends up being around 1060px on my display.

Training & Events box

I also think the "Trainings and events" sidebar could be wider — 50% (instead of the default 30%) seems to strike a much better balance. It's got an ID, so that should be as easy as sticking some custom CSS in with:

#features { width: 50%; }

30%

image

50%

image

Bulleted lists

Oh, last but not least: The new theme shows unordered lists with no bullet style at all, just an indent. It's arguably cleaner, but it does change the look of many elements. Including the two date lines in the training & events box. (And makes their indentation kind of excessive, considering the <dd> they're wrapped in already has a huge indent.)

Old theme

image

New theme

image

@ferdnyc
Copy link

ferdnyc commented May 20, 2024

Bulleted lists

Oh, last but not least: The new theme shows unordered lists with no bullet style at all, just an indent. It's arguably cleaner, but it does change the look of many elements. Including the two date lines in the training & events box. (And makes their indentation kind of excessive, considering the <dd> they're wrapped in already has a huge indent.)

Ah, I see now that that part, at least, is not the theme, as it's done locally in the templates/style.html file. Neeeever mind, then.

@@ -0,0 +1,4 @@
<style>
ul {list-style: none;}
li {margin: 5px 0};
Copy link

Choose a reason for hiding this comment

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

CSS syntax nit, and because the theme measurements are all in ems:

Suggested change
li {margin: 5px 0};
li {margin: 0.4em 0;}

<style>
ul {list-style: none;}
li {margin: 5px 0};
</style>
Copy link

@ferdnyc ferdnyc May 20, 2024

Choose a reason for hiding this comment

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

Suggested change
</style>
@media (min-width: 46em) {
#features {width: 50%;}
}
</style>

(To widen the trainings & events sidebar. At 46em the width becomes 100%, messing with that looks weird.)

@samjirovec
Copy link
Contributor Author

@ferdnyc Great callouts. Implemented those changes you suggested. I think that definitely makes that training section look a lot nicer 🙂

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Looking great, and seems to be a big improvement! Now the box also doesn't make the next heading look awkward anymore either.

Big fan of this, thanks a lot for tackling it!

@The-Compiler The-Compiler enabled auto-merge (squash) May 21, 2024 13:40
@The-Compiler The-Compiler merged commit cbf6bd9 into pytest-dev:main May 21, 2024
27 checks passed
@The-Compiler
Copy link
Member

@nicoddemus @bluetech What do you think about backporting this to 8.2.x so it appears on pytest.org already?

@bluetech
Copy link
Member

I think it's good to backport

@nicoddemus nicoddemus added the backport 8.2.x Apply on merged PRs, backports the changes to the 8.2.x branch label May 21, 2024
@nicoddemus
Copy link
Member

Agreed, backporting now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 8.2.x Apply on merged PRs, backports the changes to the 8.2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants