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

ENH: Sphinx-basic-ng as the base instead of pydata-sphinx-theme #456

Closed
wants to merge 11 commits into from

Conversation

AakashGfude
Copy link
Member

@AakashGfude AakashGfude commented Jan 10, 2022

Using sphinx-basic-ng as the base theme instead of pydata-sphinx-theme as discussed in executablebooks/meta#576 .

Some of the ongoing tasks:-

Will update this thread with pics of ongoing development.

cc: @mmcky @choldgraf @chrisjsewell @pradyunsg

@AakashGfude AakashGfude marked this pull request as draft January 10, 2022 06:53
@chrisjsewell
Copy link
Member

Directly Copying over some necessary bootstrap scss styles

@AakashGfude I would definitely check out https://github.com/executablebooks/sphinx-design/tree/main/style, which copied some bootstrap v5, BUT importantly prefixes all CSS classes (in that case with sd-)

@choldgraf
Copy link
Member

choldgraf commented Jan 11, 2022

Thanks @AakashGfude for opening this one up so that we can provide input as you go along. A few quick questions:

  1. Is there anything specifically that you'd like feedback on now?
  2. Before we go too deep down this rabbit hole, we should assess what kind of impact this will have on maintainability, vs. inheriting from the pydata theme. @AakashGfude I would be curious to hear your thoughts on what kind of impact you think this will have, now that you've had a chance to dig into the code a bit
  3. Just wanted to flag that we have a few PRs open that will touch the CSS and HTML, so this might go out of date somewhat soon.

@AakashGfude
Copy link
Member Author

AakashGfude commented Jan 11, 2022

https://github.com/executablebooks/sphinx-design/tree/main/style

@chrisjsewell sweet. Do you want me to setup this extension here, and use its inherited css styles instead of creating a copy for sphinx-book-theme? Sounds like a good idea. Some extra styles which are needed, I will copy over.

@AakashGfude
Copy link
Member Author

AakashGfude commented Jan 11, 2022

  1. Is there anything specifically that you'd like feedback on now?

Thanks @choldgraf. Pydata-sphinx-theme had webpack to process sass, js and handle npm modules. We are processing sass, js here using web-compile. But we are not installing npm modules. I am going forward with having some third-party libraries like fontawesome(scss and webfonts), stored locally. scss included in src folder and webfonts included in static folder. Instead of using npm to load and include it. Do you think we should go ahead with that?

https://github.com/executablebooks/sphinx-design as mentioned by @chrisjsewell has some bootstrap classes already, so I will just use that as the base design plugin, and add styles on top of that as needed. But while keeping a prefix like sbt to the class name.

At present, I am trying to steer away from webpack and npm modules as long as I can. Did you picture this extension having such js bundlers with this refactor?

  1. Before we go too deep down this rabbit hole, we should assess what kind of impact this will have on maintainability, vs. inheriting from the pydata theme. @AakashGfude I would be curious to hear your thoughts on what kind of impact you think this will have, now that you've had a chance to dig into the code a bit

I still dont have the complete picture. At present it looks maintainable. Although, I have to copy over some code from pydata-sphinx-theme.Till now mostly for toc.

I wil keep this point in mind and update as I go. Significant amount of pydata-sphinx-theme being copied here for the whole theme to work is expected right?

@choldgraf
Copy link
Member

Hey @AakashGfude - a few quick thoughts:

Instead of using npm to load and include it. Do you think we should go ahead with that?

I was actually thinking we'd switch over to using NPM/webpack by re-using @pradyunsg's sphinx-theme-builder: https://sphinx-theme-builder.readthedocs.io/en/latest/

That's a theme build system that installs its own nodejs when you install it locally, so the user doesn't have to worry about installing it on their own. It also has its own command line to facilitate the compilation steps that call npm (stb compile). We don't have to use this tool, but if it helps standardize our theme's structure with other themes, lets us use a web-native stack without incurring too much technical debt, and lets us use some community standards for packaging web assets with the theme, then I'd be +1 on doing this (the pydata theme started using this a month ago and it has been nice).

Till now mostly for toc.

I think that if we're going to base this theme on its own, we should start from scratch and build up the theme structure with the minimal extra CSS etc that is needed. Rather than copy/pasting everything from the PyData Theme, it might be useful to instead see if we can re-create the same look and feel but without as much CSS or HTML complexity. A good goal to shoot for is "re-create the same theme experience, but with as few lines of code + dependencies as possible". Does that make sense?

Some of these might be things that we can improve iteratively, before a full theme migration happens. Maybe we can discuss and identify a few things to improve as you start to make progress?

Although, I have to copy over some code from pydata-sphinx-theme.Till now mostly for toc.

Yep - adding the code for toggle-buttons to the left ToC will likely require us to take on some of that code. We might be able to re-use or copy over some of @pradyunsg's code for the same functionality here: https://github.com/pradyunsg/furo/blob/main/src/furo/navigation.py

@AakashGfude
Copy link
Member Author

I was actually thinking we'd switch over to using NPM/webpack by re-using @pradyunsg's sphinx-theme-builder: https://sphinx-theme-builder.readthedocs.io/en/latest/

I was looking through this too. Worth giving a try I think, and using npm modules will be nice. So, I will remove web-compile and integrate this instead and give it a shot.

I think that if we're going to base this theme on its own, we should start from scratch and build up the theme structure with the minimal extra CSS etc that is needed. Rather than copy/pasting everything from the PyData Theme, it might be useful to instead see if we can re-create the same look and feel but without as much CSS or HTML complexity. A good goal to shoot for is "re-create the same theme experience, but with as few lines of code + dependencies as possible". Does that make sense?

Yup, totally makes sense.

Yep - adding the code for toggle-buttons to the left ToC will likely require us to take on some of that code. We might be able to re-use or copy over some of @pradyunsg's code for the same functionality here: https://github.com/pradyunsg/furo/blob/main/src/furo/navigation.py

Sounds good, I think we can narrow down the web scraping from context["toctree"] and context["toc"] variables, for this particular theme from pydata-sphinx-theme code. Will contrast it with the furo code.

@pradyunsg
Copy link
Member

pradyunsg/furo@main/src/furo/navigation.py

Note that this is closely coupled with:

https://github.com/pradyunsg/furo/blob/d6b217f8bb021f137928aa30f1b41e829e87d73f/src/furo/assets/styles/components/_sidebar.sass#L93

@choldgraf
Copy link
Member

@AakashGfude in the name of iterative progress rather than single gigantic re-writes, what do you think about starting off with a modular PR to use sphinx-theme-builder? If we can keep each diff as modular as possible, it'll make it easier to take steps forward rather than have one gigantic change. Does that make sense? (I am happy to have a quick chat about this to discuss if you like)

@AakashGfude
Copy link
Member Author

AakashGfude commented Jan 13, 2022

what do you think about starting off with a modular PR to use sphinx-theme-builder?

Modular diff sounds good. I will replace web-compile with sphinx-theme-builder and create a PR.

@choldgraf
Copy link
Member

@AakashGfude OK cool - I'll merge in a couple of the open PRs since those will change the HTML structure a little bit, that way we won't create a bunch of rebase headaches

@choldgraf
Copy link
Member

By the way - here's the commit where @pradyunsg migrated the pydata theme to use the theme builder: pydata/pydata-sphinx-theme#514

@AakashGfude AakashGfude changed the title ✨ NEW: Sphinx-basic-ng as the base instead of pydata-sphinx-theme[ in development] ENH: Sphinx-basic-ng as the base instead of pydata-sphinx-theme Jan 26, 2022
@choldgraf
Copy link
Member

I'm gonna close this one as it is now super out of date, and our current strategy is to instead minimify this theme and build it as a thin layer on top of the pydata theme infrastructure

@choldgraf choldgraf closed this Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

4 participants