Improve book theme modularity and standardize HTML structure

AakashGfude commented 2 years ago

Problem statement

Our sphinx-book-theme has a sub-optimal underlying configuration, which makes it unnecessarily difficult to maintain, extend, and sub-theme (we care about sub-theming so that things like the quantecon theme can use the book theme as a base).

Proposed solution

Update our book theme's structure so that it uses the sphinx-basic-ng theme for its basic HTML structure. This would will give us more control, flexibility, readability and reduce duplication.

It's unclear whether this should be done at the parent theme level (pydata-sphinx-theme), or if instead we should just directly sub-theme the sphinx-basic-ng theme.

Extra Background

sphinx-basic-ng is a modernized skeleton for sphinx themes. And aims to act as a base for more complicated and opinionated themes required by projects.

pydata-sphinx-theme uses the "basic" sphinx theme at present, as its base.

Our sphinx-book-theme is a sub-theme of the pydata sphinx theme.

Implementation guidance

Here's a nice mock-up of the overall theme structure that @chrisjsewell wrote up for inspiration:

Actions to take

[ ] Build a prototype of the sphinx-book-theme that directly uses the sphinx-basic-ng theme, but results in a nearly similar end-product (maybe with some modifications per the mock-up above)
[ ] After 2-4 weeks, decide whether the result is worth continuing, or if there is enough extra complexity that it would be worth still using the pydata-sphinx-theme (and modify that theme to use the sphinx-basic-ng theme)
[ ] ...decide what else need to do as we learn more

cc: @choldgraf @mmcky

ref: https://github.com/executablebooks/sphinx-book-theme/issues/416 #279

mmcky commented 2 years ago

@AakashGfude thanks for launching this effort. It will be great to kickstart defining the theme infrastructure for jupyter-book more cleanly. I made a few edits to the above block to identify tasks etc.

AakashGfude commented 2 years ago

Thanks, @mmcky, for defining action points clearly.

chrisjsewell commented 2 years ago

Heya, I would note to some extent this is a duplicate of #279. Also, I'm not sure why we want to look at adapting pydata-sphinx-theme; if we are to use sphinx-basic-ng, then sphinx-book-theme should use this as a base directly, not via pydata-sphinx-theme. I.e. We should be looking at re-writing sphinx-book-theme with sphinx-basic-ng as its base, including the modular plugin system I have already discussed for side/top elements (download buttons, launch buttons, etc)

choldgraf commented 2 years ago

I think we can use this issue as a more action-focused issue to start exploring the ideas in https://github.com/executablebooks/meta/issues/279 (which is long and kind-of unweildy to parse).

Regarding sub-themeing pydata-sphinx-theme vs. directly sub-theming sphinx-basic-ng. I can see two sides, I'm not sure which is the best path forward:

An argument for continuing to use the pydata theme and modifying it to use `sphinx-basic-ng`

We can re-use whatever components are built into that theme. This may reduce our maintenance burden by sharing more infrastructure with another project that has a decent amount of usage and development. For example:

General typography, styling, and spacing
Left navbar dropdowns
Version dropdown
Things like white/black mode

And more generally, given that many of us are using both the pydata theme + the book theme for different projects, this might be a way to improve both projects with a single action rather than having to implement things in both themes at once. (this was one of the main reasons we started off by sub-theming the pydata theme directly).

Steps we'd take to implement this

In the pydata theme:

Re-work the theme to use the sphinx-basic-ng theme as a base
Start from scratch and re-create the same theme structure / style by refactoring our HTML and adding the necessary SCSS
In the process, update the HTML classes to use Bootstrap 5 instead of Bootstrap 4
Add a prefixer to the CSS processor so that our bootstrap rules no longer clash with Sphinx-native class names
Probably remove the use of Bootstrap JS wherever we can, and instead just use it for the CSS helper classes

Then in the book theme

Go through the same process, but re-using the new version of the pydata theme.

An argument for using the sphinx-basic-ng theme directly

We'd have more control over the entire theme stack, and wouldn't have to include infrastructure that special-cases the pydata theme (e.g., if we wanted to include or exclude certain configuration). There'd also be a tighter loop of improvements -> releases, since we wouldn't need to wait for the pydata theme to release first. This could also reduce complexity and confusion because maintainers of the book theme wouldn't need to also consider the pydata theme in their development.

Steps we'd take to implement this

Do everything in the above scenario but for the book theme instead of the pydata theme
Add our own implementation of all the features/styles that we currently inherit from the pydata theme (e.g., the list above and probably other stuff too)

Suggested action

How about something like the following:

Agree on a short-term direction, and give a shot at implementing the theme changes needed to make it happen (AKA, decide if we wanna try the following in either the book theme or the pydata theme)
See how much of our current theme we can re-create, starting with the sphinx-basic-ng theme, and with the minimal number of additions possible (e.g., we can try without Bootstrap and see how much CSS complexity this adds, and decide later if we should add Bootstrap to make our lives easier)
Time-box ourselves at like 2 weeks to re-assess the complexity we're running into when that happens. We can re-assess that once we've had a chance to try implementing something and have a better feel for how much work it'll be.

It sounds like @chrisjsewell is in favor of using the sphinx-basic-ng theme directly...do @AakashGfude or @mmcky agree / object? How do you two imagine the QuantEcon theme relating to the work here? Would you sub-theme the new sphinx-book-theme? If that's the case (and if we imagine other people creating their own sub-themes from the book theme in generla), it might be worth trying out these changes in the book theme rather than the pydata theme just to keep things simple.

mmcky commented 2 years ago

Without being very knowledgeable of css/html myself I don't think I hold the strongest vote on this issue :-). But I would vote for the flatter inheritance structure -- I find large nested structures more difficult to debug and get to an understanding of where issues may be.

re quantecon-book-theme I would like to see it move to inheriting from whatever the base is for jupyter-book themes rather than sphinx-book-theme. That way we have items like margin notes supported by default in jupyter-book themes and it is up to the new theme to override a style etc.

AakashGfude commented 2 years ago

At present in quantecon-book-theme we re-use python code of sphinx-book-theme and inherit pydata-sphinx-theme for HTML. Though, @DrDrij had suggested making it a standalone theme without a complicated inheritance chain. But we realized we were re-writing a lot of the similar code, so ended up inheriting it.

In addition to the points above, my two cents:

Reason to use pydata-sphinx-theme

sphinx-basic-ng i believe is meant to be a very minimal skeleton structure only. It does not have any javascript or anything fancy and just creates a scaffolding. So, updates or changes to its code will be very minimal? (mostly only if we are changing the structure of the page itself.) . pydata-sphinx-theme adds a base flavor to it, in terms of adding translations, toc handling, launch button, margin directive, etc. If we directly inherit from sphinx-basic-ng we will have to copy it over to each theme. Although @chrisjsewell is suggesting to transfer these in modular plugins. The tradeoff will be the increase in the number of repositories we will have to maintain and the number of dependencies in each project.

Reason to use sphinx-basic-ng directly

Lesser overwrites and trying to bend the code to meet your requirements. As most of the functionality and specific styles will be in the theme package itself.

I agree with @choldgraf we should start off with removing bootstrap and writing our own CSS, as bootstrap is quite opinionated sometimes and has a lot of !important CSS rules, which makes overriding cumbersome. We can also brainstorm on using some other lightweight CSS framework.

chrisjsewell commented 2 years ago

pydata-sphinx-theme adds a base flavor to it, in terms of adding translations, toc handling, launch button, margin directive, etc

I would note that all four of these aspects are implemented in sphinx-book-theme, not inherited from pydata-sphinx-theme

chrisjsewell commented 2 years ago

Although @chrisjsewell is suggesting to transfer these in modular plugins. The tradeoff will be the increase in the number of repositories

It should be a modular system, but I'm not suggesting putting all the plugins in different repos, all the default ones used by sphinx-book-theme, would be in the sphinx-book-theme

chrisjsewell commented 2 years ago

This may reduce our maintenance burden by sharing more infrastructure with another project

I just feel that has not really been the case, since in sphinx-book-theme, we override nearly every aspect of the pydata-theme: the html templates, the css, the translations.

we should start off with removing bootstrap and writing our own CSS,

Yep definitely. If anything we should be inheriting from furo, which has a structure and css styling much more inline with sphinx-book-theme, than pydata-sphinx-theme

AakashGfude commented 2 years ago

I would note that all four of these aspects are implemented in sphinx-book-theme, not inherited from pydata-sphinx-theme

Mostly true. But, the implementations in sphinx-book-theme for like toc rely on pydata-sphinx-theme base functions and some functions like generate_nav_html we use directly from from pydata-sphinx-theme in quantecon-book-theme.

AakashGfude commented 2 years ago

There are a few other small things which sphinx-book-theme inherits from pydata-sphinx-theme like fontawesome and some helper context functions. Which can be copied over to sphinx-book-theme nonetheless.

All in all, from a quantecon-book-theme's perspective (which should be true for other themes as well), it would have been ideal to inherit from a project which has base scafollding sorted out, and also handled functionalities like toc, directives like margin, translations etc on a basic level, so we just build on top of that. Like for example, the generate_nav_html function in pydata-sphinx-theme which both sphinx-book-theme and quantecon-book-theme uses to create their own styling.

mmcky commented 2 years ago

All in all, from a quantecon-book-theme's perspective (which should be true for other themes as well), it would have been ideal to inherit from a project which has base scafollding sorted out

Completely agree with this. If sphinx-basic-ng doesn't provide that I think we need a base jupyter-book layer to minimise any upstream edits to themes. It would be great if we could just have a lightweight theme layer where you adjust styles and a base that incorporates the complex things like hash generated theme files etc., modularity etc.

But I think any end-user theme should inherit from this base. So sphinx-book-theme and quantecon-book-theme would inherit from a common base and exist adjacent to one another (from an inheritance perspective)

Basically -- it would be great if I could write a theme without the advanced knowledge of css and html currently required.

choldgraf commented 2 years ago

If anything we should be inheriting from furo, which has a structure and css styling much more inline with sphinx-book-theme, than pydata-sphinx-theme

I was wondering this as well, it is an interesting idea! The only downside is that I'm not sure that @pradyunsg would make any promises regarding stability etc of Furo, I'm not sure he wants people to sub-theme his theme at all 😅, but I'm curious what he or others think...maybe it'd save a significant amount of time and unnecessary duplication of CSS rules? (e.g., white/black themes).

and some functions like generate_nav_html we use directly from pydata-sphinx-theme in quantecon-book-theme.

This makes me wonder if a function like this would be useful in the sphinx-basic-ng theme (generate the TOC and return it as a string of HTML) rather than in pydata-sphinx-theme. In fact I believe there's already an issue to discuss this: https://github.com/pradyunsg/sphinx-basic-ng/issues/16 and @pradyunsg has a PR that gets partway there: https://github.com/pradyunsg/sphinx-basic-ng/pull/25

But I think any end-user theme should inherit from this base. So sphinx-book-theme and quantecon-book-theme would inherit from a common base and exist adjacent to one another (from an inheritance perspective)

I had imagined that the sphinx-book-theme would be this "base theme", since its styling is relatively unopinionated and it defines the basic elements like margins, buttons, etc. In my mind the inheritance chain would be:

sphinx-basic-ng -> provides the base HTML scaffolding and some very lightweight CSS
either sphinx-book-theme or pydata-sphinx-theme -> provides CSS variable structure, nice-looking but unopinionated typography and styles, and the core feature set that would be useful across all of the sub-themes
sphinx-book-theme -> provides book-specific components like margins, topbar buttons, etc
other sphinx themes would inherit from the sphinx-book-theme, and could over-ride styles via their own HTML templates and CSS over-rides

I am leaning towards thinking we should just give a shot at:

Building the sphinx-book-theme as a direct sub-theme of sphinx-basic-ng
Working with @pradyunsg to upstream or review changes in the sphinx-basic-ng theme if there's anything with the theme structure that'd be useful to have
In 2 weeks (maybe 4 weeks if @AakashGfude is planning on taking holiday time off!) assessing how complex the end result looks like it'll be (e.g., will there be a ton of extra HTML / CSS we'll be responsible for maintaining?)
Decide then whether to keep moving in that direction, or to consider doing the theme inheritance work on the pydata-sphinx-theme instead.

AakashGfude commented 2 years ago

Looks like a good action plan @choldgraf. So, should we wait for that PR https://github.com/pradyunsg/sphinx-basic-ng/pull/25/files to be merged before starting off or copy over the functionality implemented in pydata-sphinx-theme and other functionalities for now and shift to sphinx-basic-ng equivalent when it is ready?

From what I see, the end role of sphinx-basic-ng is to provide HTML scaffolding, lightweight CSS, and some python code which returns data structures (dictionaries, etc) to be used by other themes.

choldgraf commented 2 years ago

As long as others think this is a reasonable plan, then I'd say we should just give a shot at doing the conversion now. Maybe it'll turn out to be relatively quick and simple, and then we only need to "copy/paste" a single Python function from the pydata-sphinx-theme that we can deprecate when https://github.com/pradyunsg/sphinx-basic-ng/pull/25/files gets in 🤷 but I think the main thing now is to reduce our uncertainty a bit by trying to make the conversion, so that we can make more informed decisions about next steps later on.

pradyunsg commented 2 years ago

As a couple of quick notes:

sphinx-basic-ng's documentation is out of date. Look at the templates to figure out the exact details of how stuff works and what the knobs are. Variables starting with theme_ are provided by the user in their conf.py file.

I'm not sure he wants people to sub-theme his theme at all 😅,

Right now, it's not even possible to create a subtheme from Furo; there's hard coded checks to prevent that.

Basically -- it would be great if I could write a theme without the advanced knowledge of css and html currently required.

This is unlikely to change; at least, not with sphinx-basic-ng.

What that is meant to do is provide reusable chunks for writing Sphinx theme templates to (a) reduce the amount of duplicated work needed for implementing things like breadcrumbs [^1] (b) provide a general framework to structure your Sphinx themes with, potentially allowing for code reuse and (c) providing an opinionated but very flexible scaffold that can be overridden to whatever degree you want (just add a few styles, or redo the CSS or redo CSS and HTML).

None of this eliminates the need to know a decent degree of CSS (things like making sidebars sticky, or preventing tables/code from overflowing the content container etc), but it should help reduce/remove some of the pain of trying to get a responsive scaffold. It removes/reduces a lot of figuring out how to implement $thing in your Sphinx theme as well, by providing dedicated components to use.

[^1]: Which is trivial to reuse when there's no existing contract with the user. If you already provide some configuration knobs for this and backwards compatibility concerns, you can override components/breadcrumbs.html to have your custom breadcrumbs template instead of the default one.

AakashGfude commented 2 years ago

Cool . I will start the conversion process, and see how we go.

chrisjsewell commented 2 years ago

Cheers @AakashGfude

choldgraf commented 2 years ago

I've updated the top comment to reflect some of our more recent conversations. @AakashGfude I also added you to the issue since you're focusing on this one. I'm happy to review and PRs or have discussion as needed! Also happy to help things on the sphinx-basic-ng side if we find places to upstream stuff.

AakashGfude commented 2 years ago

Thanks @choldgraf

executablebooks / meta