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

Switch Theme To Furo #2626

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

Switch Theme To Furo #2626

wants to merge 31 commits into from

Conversation

GrahamSH-LLK
Copy link
Contributor

@GrahamSH-LLK GrahamSH-LLK commented Apr 2, 2024

Furo

  • Is a noticeably better looking theme than RTD
  • Waaaay easier to navigate sidebar
  • Table of contents on the side (currently unused space on rtd-theme)
    • Makes viewing and finding content on long pages WAY easier
  • Is significantly more responsive on small screens
  • Has a built in and good looking dark mode
  • Is more maintained than RTD-theme (often taking months to release new version that are compatible with sphinx)
  • More modern looking admonitions
  • Better colors and font
  • Used by CTRE :)

image
image
image

@GrahamSH-LLK GrahamSH-LLK marked this pull request as draft April 2, 2024 11:08
@GrahamSH-LLK GrahamSH-LLK marked this pull request as ready for review April 3, 2024 01:05
@jasondaming
Copy link
Member

I think @Daltz333 might be the best judge if and how this change could be made. As is there are multiple things I don't like about the current implementation:

  • The upper left logo looks bad and is far too elongated. I think a lot of effort has gone into the previous one and it should be very similar to that:
    image
  • Is there any way we can keep the font sizing more similar. It seems to have made many things bigger which throws some things off. For example the "FIRST Robotics Competition Control System" on the home page now is taking two line as opposed to 1.
  • On my screen it seems like it has semi centered the page. More gap on the right than left. I think I prefer left justified but not a hill I would die on maybe I am just resistant to change.
  • The rounded search box looked better to me.
  • The scroll on the left hand side only moves the topics and not the logo above. I don't think we need to see the logo the whole time.
  • It seems like the spacing and size of the items on the left is larger and you now need to scroll forever to reach the bottom.
  • Do all the links have to be underlined? That seems like it sets them even further apart from the text where as the old theme blended better.

Overall I like many of the features this brings I think we just need to have a deeper discussion on many of the "fine" points.

@GrahamSH-LLK
Copy link
Contributor Author

I think @Daltz333 might be the best judge if and how this change could be made. As is there are multiple things I don't like about the current implementation:

  • The upper left logo looks bad and is far too elongated. I think a lot of effort has gone into the previous one and it should be very similar to that:
    image
  • Is there any way we can keep the font sizing more similar. It seems to have made many things bigger which throws some things off. For example the "FIRST Robotics Competition Control System" on the home page now is taking two line as opposed to 1.
  • On my screen it seems like it has semi centered the page. More gap on the right than left. I think I prefer left justified but not a hill I would die on maybe I am just resistant to change.
  • The rounded search box looked better to me.
  • The scroll on the left hand side only moves the topics and not the logo above. I don't think we need to see the logo the whole time.
  • It seems like the spacing and size of the items on the left is larger and you now need to scroll forever to reach the bottom.
  • Do all the links have to be underlined? That seems like it sets them even further apart from the text where as the old theme blended better.

Overall I like many of the features this brings I think we just need to have a deeper discussion on many of the "fine" points.

Check out the sidebar changes I just pushed. It looks way better to me. The rest I'd like to get some more opinions before changing, as I've heard very different takes on them from different people.

@sciencewhiz
Copy link
Collaborator

sciencewhiz commented Apr 6, 2024

On the topic of looks, the people who's opinion matters most are @Kevin-OConnor (FiRST) and @arbessette (WPI). I suspect they're both busy until after championships.

@Kevin-OConnor
Copy link
Contributor

Yep, pretty swamped through Champs and recovering the week after. Just set myself a calendar notification for the 29th to look at this. Sorry for the delay!

@Kevin-OConnor
Copy link
Contributor

Kevin-OConnor commented May 1, 2024

Overall I think this looks much better than when I first looked at it. Couple comments:

  • On my laptop screen the sidebar looks a little odd. I think it's a result of the overall content being centered, but the padding on the left that's the dark blue sidebar color stands out much more than the white/black padding on the right. I've included an image below. I'm not sure what exactly to do different though.
  • In dark mode the green/teal headers on the homepage look a little ugly to me. I don't think this should be a blocker to merging, this is always something that could be changed later.

I like the "On this page" mini table of contents that's part of this. I like the larger text size for the title, I think it helps convey the hierarchy a little better, the text sizes are pretty close between titles and various heading levels in the current theme.
image

@GrahamSH-LLK
Copy link
Contributor Author

Overall I think this looks much better than when I first looked at it. Couple comments:

  • On my laptop screen the sidebar looks a little odd. I think it's a result of the overall content being centered, but the padding on the left that's the dark blue sidebar color stands out much more than the white/black padding on the right. I've included an image below. I'm not sure what exactly to do different though.

It doesn't look particularly weird to me on my laptop, but I can see how it would to others/on different resolutions. I'm not entirely sure how to fix this either- we could reduce the padding, but I feel like that might look worse.

  • In dark mode the green/teal headers on the homepage look a little ugly to me. I don't think this should be a blocker to merging, this is always something that could be changed later.

I can definitely change colors. Which color specifically?

I like the "On this page" mini table of contents that's part of this. I like the larger text size for the title, I think it helps convey the hierarchy a little better, the text sizes are pretty close between titles and various heading levels in the current theme.

image

@GrahamSH-LLK
Copy link
Contributor Author

GrahamSH-LLK commented Jun 12, 2024

Before merging I would like to get somebody who understands one of the RTL language(s?) to make sure I didn't break anything in the change. I tried to keep everything looking like it did on the old version, but it is very possible that I missed something.

@sciencewhiz
Copy link
Collaborator

Before merging I would like to get somebody who understands one of the RTL language(s?) to make sure I didn't break anything in the change.

@AustinShalit @Starlight220

@Starlight220
Copy link
Member

Is there a way to build the Hebrew translation with the changes here? I can see if it's broken, but I'm not familiar enough with RTD/CSS/etc to know in advance.

@GrahamSH-LLK
Copy link
Contributor Author

Is there a way to build the Hebrew translation with the changes here? I can see if it's broken, but I'm not familiar enough with RTD/CSS/etc to know in advance.

here you go: https://he.grahamsh.com/

@@ -31,9 +31,10 @@ def do(app: Sphinx, exception: Union[Exception, None]) -> None:
print("Running custom post processing")

# The two fontawesome functions must run in order
cleanup_fontawesome_css(app)
cleanup_fontawesome_font_files(app)
switch_to_system_fonts(app)
Copy link
Member

Choose a reason for hiding this comment

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

Does furo use system fonts by default?

@Starlight220
Copy link
Member

Starlight220 commented Jun 13, 2024

@GrahamSH-LLK thanks for the build!

Some panel buttons on the homepage lost the framed link: Screenshot_20240613-065956.png
It seems to be that way with all the Hebrew buttons on that page; I'm starting to suspect that a lot got broken.

The headings sidebar opens properly, but the TOC sidebar doesn't open.

Another note (not only RTL), I'm not sure I'm a fan of the blockquoting here:
Screenshot_20240613-070746.png

This might be a problem in the source itself (note the difference in capitalization of the language labels):
Screenshot_20240613-071134.png

Screenshot_20240613-071118.png

@GrahamSH-LLK
Copy link
Contributor Author

@GrahamSH-LLK thanks for the build!

Some panel buttons on the homepage lost the framed link: Screenshot_20240613-065956.png It seems to be that way with all the Hebrew buttons on that page; I'm starting to suspect that a lot got broken.

Believe it or not, those buttons seem to be missing on stable Hebrew too. I'm not sure if this is intentional, but it's not a regression in this PR.

The headings sidebar opens properly, but the TOC sidebar doesn't open.

That was just fixed (see https://he.grahamsh.com)

Another note (not only RTL), I'm not sure I'm a fan of the blockquoting here: Screenshot_20240613-070746.png

What do you dislike exactly?

This might be a problem in the source itself (note the difference in capitalization of the language labels): Screenshot_20240613-071134.png

Screenshot_20240613-071118.png

That must be a mistake from an earlier PR of mine where sphinx tabs were migrated. I'll send a PR to make it consistent soon.

@sciencewhiz
Copy link
Collaborator

Believe it or not, those buttons seem to be missing on stable Hebrew too. I'm not sure if this is intentional, but it's not a regression in this PR.

wpilibsuite/frc-docs-translations#38

@Starlight220
Copy link
Member

Buttons: bummer it's an upstream limitation, but oh well.

TOC sidebar: fixed; thanks!

Changelog blockquote: I'm not entirely a fan of the seemingly-arbitrary blockquoting of nested lists. Why not keep it without blockquotes? Just seems a bit weird imo that part of the changelog is quoted and some isn't, but it's probably a matter of taste so I won't object.

Tab labels: sounds good.

Overall, LGTM!

Copy link
Collaborator

@sciencewhiz sciencewhiz left a comment

Choose a reason for hiding this comment

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

sphinx-rtd-theme should be able to be removed from pyproject.toml

@GrahamSH-LLK
Copy link
Contributor Author

sphinx-rtd-theme should be able to be removed from pyproject.toml

removed.

@sciencewhiz
Copy link
Collaborator

Changelog blockquote: I'm not entirely a fan of the seemingly-arbitrary blockquoting of nested lists. Why not keep it without blockquotes? Just seems a bit weird imo that part of the changelog is quoted and some isn't, but it's probably a matter of taste so I won't object.

I agree, that looks weird. That doesn't happen here with nested lists: https://sphinx-themes.org/sample-sites/furo/kitchen-sink/lists/

@sciencewhiz
Copy link
Collaborator

sciencewhiz commented Jun 18, 2024

This is why: pradyunsg/furo#190

Should probably find places with 4 space indents and fix, seems like it could show up many places.

@sciencewhiz
Copy link
Collaborator

There's conflicts to resolve

@TheTripleV
Copy link
Member

@sciencewhiz sciencewhiz added this to the 2025 milestone Aug 3, 2024
@rzblue
Copy link
Member

rzblue commented Aug 23, 2024

The js files are being added to the html past the content, so they're not available at the js entrypoint in the article. Wrapping the entrypoint in a load event listener fixes it
e.g.

<script>
   addEventListener("load", (event) => {
      flywheel_pid = new FlywheelPIDF("flywheel_feedforward", "feedforward");
   });
</script>

@sciencewhiz
Copy link
Collaborator

Depends on pradyunsg/furo#795

@sciencewhiz
Copy link
Collaborator

Depends on #2729

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.

7 participants