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 pydata theme to 0.10 #597

Merged
merged 75 commits into from
Oct 26, 2022
Merged

Update pydata theme to 0.10 #597

merged 75 commits into from
Oct 26, 2022

Conversation

AakashGfude
Copy link
Member

@AakashGfude AakashGfude commented Jul 28, 2022

This PR updates the theme to synchronize with pydata-sphinx-theme~=0.10. The aim here is to just refactor the existing code to harmonize with the latest pydata. So, no new features will be added. Although a lot of CSS rework would be done here, apart from HTML and CSS.

  • CSS rework to remove redundancy and mimic the existing sphinx-book-theme style, and use variables wherever possible.
  • Handling footer-content in pydata-sphinx-theme. Latest pydata although following most of the skeleton of sphinx-basic-ng does not seem to have a section for footer-content. Which is needed in sphinx-book-theme :

footer-content in sbt:
Screen Shot 2022-07-28 at 11 12 22 am

for reference in s-b-ng:

Screen Shot 2022-07-28 at 11 13 52 am

We can probably just use a hacky one in this PR and update it in relevant sources in upcoming cycles.

EDIT: Done in pydata/pydata-sphinx-theme#861

  • Javascript not broken with updated HTML.
  • restructuring HTML to inherit as much as possible.

fixes #575

@choldgraf
Copy link
Member

Just a note that some of this has already been done in the PR below, but if you think it's better to start a fresh PR I am +1. Will link below if it's useful

I think we should use this as an opportunity to simplify and reduce as much as we can...there were many special-cases in CSS, HTML structure, etc that we had in this theme, that we have since upstreamed to the pydata theme, so we should be able to slim things down here a lot more.

@AakashGfude
Copy link
Member Author

Just a note that some of this has already been done in the PR below, but if you think it's better to start a fresh PR I am +1. Will link below if it's useful

* [[WIP] Update pydata theme #574](https://github.com/executablebooks/sphinx-book-theme/pull/574)

I think we should use this as an opportunity to simplify and reduce as much as we can...there were many special-cases in CSS, HTML structure, etc that we had in this theme, that we have since upstreamed to the pydata theme, so we should be able to slim things down here a lot more.

Ahh yes, Thank you for linking the PR.

@choldgraf
Copy link
Member

@AakashGfude let's try to add in the content footer in the PyData theme as an empty template for now. That way sub themes can override it even if it's not used in the PyData theme.

pyproject.toml Outdated
"sphinx>=3,<5",
"pydata-sphinx-theme~=0.8.0",
"sphinx>=3,<6",
"pydata-sphinx-theme~=0.10.0rc1",
Copy link
Member

Choose a reason for hiding this comment

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

I'd try rc2 - that makes some more changes

@AakashGfude
Copy link
Member Author

@choldgraf, I tried introducing a button for the theme mode and added a few CSS changes. But realized that it would need a lot of CSS updates to work seamlessly. So, I think it will be best to do that in a new PR. For now, I have set the default_mode config to "light". And also changing the HTML dataset attributes on page load in JS. To update the localStorage, in case it had a previous value. As setting the default_mode config does not update the localStorage.

Full-width cells should be fine now. And margin style anomaly I could not detect. Probably was fixed by some other changes.

Let me know how it looks now, and thank you for taking some time out for this.

@choldgraf
Copy link
Member

choldgraf commented Oct 20, 2022

A few more visual bugs. Can you please go through the kitchen sink and the theme-specific elements page and confirm that everything looks normal. That is the easiest way to spot these bugs. I'd like to get this PR merged quickly and I know that we will need to keep iterating, but I at least want to avoid introducing an obvious visual bug into our docs.

  • Admonitions still have a weird margin space: image
  • Code blocks with a title have extra margin at the bottom: image
  • Sidebar items (and I think rubrics too?) have an extra white space to the left as well, look at the underline of the title: image

@AakashGfude
Copy link
Member Author

@choldgraf thanks for reporting this, and looking through all this patiently. I think the ones notified in the last comment have been resolved with the latest push. I will look through those pages for any visual anomalies.

@choldgraf
Copy link
Member

Sounds good @AakashGfude - let me know when you've taken a close look and think it is ready to merge, and then I can give a final look-over.

@AakashGfude
Copy link
Member Author

@choldgraf from my inspection of the pages, I think this PR looks good.

Just pointing out some visual changes, which are inherited from pydata-sphinx-theme. The changes inherited looks better to me.

Left: original right: this PR

  1. space between caption and legend.

Screen Shot 2022-10-23 at 5 46 50 pm Screen Shot 2022-10-23 at 5 46 33 pm

  1. The position of caption for list tables are above the list tables.

Screen Shot 2022-10-24 at 8 45 24 pm Screen Shot 2022-10-24 at 8 26 12 pm

  1. double ticks are bordered.

Screen Shot 2022-10-24 at 8 51 19 pm Screen Shot 2022-10-23 at 2 05 40 pm

@stevepiercy
Copy link
Contributor

Sorry to jump in late with feedback. I like the improvements and customizations made so far.

I found a few more discrepancies that are important to our documentation projects.

The search box hint text is very light and difficult for me to read.

Original

search-original

Proposed

search-new

The seealso admonition's icon and color changed to the default info admonition. I prefer the green curvy arrow icon.

Original

seealso-orig

Proposed

seealso-new

The contents menu no longer highlights the final heading of a page when scrolled to the end.

Original

contents-menu-orig

Proposed

contents-menu-new

The hint and important admonitions have changed from yellow to green. I think they should be reverted to yellow.

Original

hint-important-orig

Proposed

hint-important-new

The tip admonition has been changed from yellow to green. I think it should be reverted.

Original

tip-orig

Proposed

tip-new

@choldgraf
Copy link
Member

Thanks @stevepiercy ! A few quick responses from me on next steps for these suggestions:

  • The search box hint text is very light and difficult for me to read.
  • The seealso admonition's icon and color changed to the default info admonition. I prefer the green curvy arrow icon.
  • The contents menu no longer highlights the final heading of a page when scrolled to the end.
    • I suspect that this is because the end section in this case is relatively short - it is highlighting a section once it is above X% of the page, and I bet it's just not high enough to trigger that here. Maybe we can tweak the UI/UX a bit, but it doesn't feel to me like a "bug" per se.
  • The tip admonition has been changed from yellow to green. I think it should be reverted.
    • This got changed in the upstream theme in order to reduce the number of unique color variables being used. If we want to use yellow we'll need to define our own color variable in both light and dark modes and apply it to these two admonition types. I feel like we could open an issue and track that one so that it doesn't block this PR.

@AakashGfude
Copy link
Member Author

Thank you for the inputs @stevepiercy, and for the replies @choldgraf. The latest push should have the search field text covered. I agree with the admonition styling being done upstream.
The highlighting on the end of the page, i reckon as mentioned by @choldgraf is subjective it seems. In my laptop, it is working fine:
Screen Shot 2022-10-25 at 11 58 50 pm

@stevepiercy
Copy link
Contributor

Thanks for the responses. I agree on the course forward on all points. I will work on the new issues next week. Full speed ahead for this PR!

@choldgraf choldgraf changed the title [WIP] Update pydata to 0.10 Update pydata theme to 0.10 Oct 25, 2022
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

OK I think that this one is ready to go - I bet that we will still need to make some tweaks over time, and I also think it'd be great to try and take another pass at removing more customizations here to just re-use the pydata theme structure as much as possible, but this is a big complex PR and it'll be out of date quickly if we merge anything in. @AakashGfude what do you think?

@AakashGfude
Copy link
Member Author

@choldgraf I agree. Let's merge this one. And continue the tweaks over time.

@AakashGfude AakashGfude merged commit 8da268f into master Oct 26, 2022
@stevepiercy
Copy link
Contributor

On second thought, I think the tip and hint admonitions are OK as green, but I think blue would be better. The color green has a universal context of "go", whereas blue's is "information".

I think important should be yellow, not green, as it is similar in purpose and context to caution.

I can see that this theme inherits the colors from pydata: https://pydata-sphinx-theme.readthedocs.io/en/stable/examples/kitchen-sink/admonitions.html

I could open an issue in the pydata theme to discuss, but I wanted to get @choldgraf's opinion first. Please let me know. Thank you!

@choldgraf
Copy link
Member

I think that makes sense to me - let's discuss in the pydata theme and see where that goes - what do you think?

@stevepiercy
Copy link
Contributor

Done! pydata/pydata-sphinx-theme#1040

@kloczek
Copy link

kloczek commented Oct 26, 2022

Any chance to tag new version soon? 🤔

@SilverRainZ
Copy link

Kindly ping, any plan to make a new release? :D

@choldgraf
Copy link
Member

I think there are some more breaking changes we'll need to make here:

My feeling is that we should get that done at least as an MVP before we release, so that we can simplify this one as much as possible and start to make it much easier to update in the future

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.

Update pydata theme to 0.10
6 participants