executablebooks / sphinx-book-theme

A clean book theme for scientific explanations and documentation with Sphinx
https://sphinx-book-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
441 stars 201 forks source link

ENH: Add search field back to sidebar #775

Closed choldgraf closed 1 year ago

choldgraf commented 1 year ago

This makes minor CSS adjustments to fix some changes made in the latest pydata theme update.

Brief summary:

Issues resolved:

I think this is ready to merge, the two failures aren't related to this PR...

choldgraf commented 1 year ago

@Gouvernathor I've got some changes in this one that are relevant to you, if you want to take a look. Let me know if this is a reasonable step forward to avoid unexpected CSS changes.

I think we do want to incorporate improved accessibility, but hopefully this PR does so without changing the defaults as much. I don't want to get into a long debate about minor design details, so please focus your comments on anything really major that is a huge problem as opposed to stuff that might be a matter of preference! (agreeing on design is always a huge challenge haha)

Gouvernathor commented 1 year ago

That sounds great, how can I test how it renders ? I tried editing the .scss files from my local install but it doesn't seem to work.

choldgraf commented 1 year ago

I think for now your best bet is to look at the rendered documentation preview from this PR.

https://sphinx-book-theme--775.org.readthedocs.build/en/775/

It will be way easier to just check that out and estimate how it would look rather than to build your docs locally with this PR.

Gouvernathor commented 1 year ago

Thanks ! That's more than enough.

As for the background color, I would prefer the former color, #121212, which is visually much darker than this new one. An even darker one would be acceptable too, for me, but a simple rollback to the previous color (which is still part of the version of book that's considered "stable") may be more acceptable for users at-large who would see no change at all.

The "back to top" and research bar changes are great. If the initiative of making the "back to top" toggleable on the base theme gets merged, it would probably be cleaner to just do that instead, though that's more of an internal concern, and it doesn't prevent merging this in the meantime.

choldgraf commented 1 year ago

Fair enough - I think keeping the old background color is less messy anyway. I'd rather not change something if we don't have a clear rationale for doing so. I've reverted that back in the last commit

choldgraf commented 1 year ago

OK going to merge this one in. I think it's a good enough improvement and is another step to unblocking folks with all the upstream updates.