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

Add panels support #641

Merged
merged 29 commits into from May 24, 2022
Merged

Add panels support #641

merged 29 commits into from May 24, 2022

Conversation

12rambau
Copy link
Collaborator

@12rambau 12rambau commented Apr 23, 2022

Fix #627

as I'm still unable to build the doc on my machine, I'll use the draft to trigger the RDT build.

WIP

@choldgraf
Copy link
Collaborator

choldgraf commented Apr 23, 2022

I'd use Sphinx Design, not Sphinx Panels (https://sphinx-design.readthedocs.io/en/latest/). The syntax is very similar though.

@12rambau
Copy link
Collaborator Author

I'd use Sphinx Design, not Sphinx Panels (https://sphinx-design.readthedocs.io/en/latest/). The syntax is very similar though.

Yep I saw your comment in the issue, as it's the same class I'll work with that for now and I'll update the lib name once everything works

@12rambau
Copy link
Collaborator Author

12rambau commented May 3, 2022

I think I now have something. I respected the structure initiated by @choldgraf and added 2 files in the extention folder. one to support sphinx-design and one for sphinx-panels fore legacy documentations.

In short you'll find in this PR:

  • 2 sets of css rules to overwrite the one provided by sphinx-design and sphinx-panels extentions
  • a new page to check the panels. I used sphinx-design for the examples
  • some small modification to the navbar coloring to avoid being overwritten by other extention

several notes:

  • the 2 extentions are fully incompatible as they both provide the :fa: role and are based on different versions of bootstrap
  • I only support panels and tabs, the other components are less comonly used and should thus be customized by the user
  • sphinx-design was easier to customize so I would prefer to use it instead of sphinx-panels
  • there are a lot of !important rules due to the order of the files in the final html.

@12rambau 12rambau marked this pull request as ready for review May 3, 2022 21:58
Copy link
Collaborator

@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.

Thanks for this improvement! I added a few suggestions.

docs/demo/kitchen-sink/web-components.rst Outdated Show resolved Hide resolved
docs/demo/kitchen-sink/web-components.rst Outdated Show resolved Hide resolved
docs/demo/kitchen-sink/web-components.rst Show resolved Hide resolved
* cards
*/

.sd-card {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that sphinx-design uses many of its own CSS variables:

https://sphinx-design.readthedocs.io/en/latest/css_variables.html

If possible, I think it would be better to control the behavior here by using --pst variables to set --sd variables, rather than coding new CSS rules. Does that make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

design-style .css is loaded after pydata-sphinx-theme one so I won't be able to overwrite anything (that's also why I need to use !important rules in lots of places). I think I can't.

  <!-- Loaded before other Sphinx assets -->
  <link href="_static/styles/theme.css?digest=fb52004a9691e5b75515" rel="stylesheet">
<link href="_static/styles/pydata-sphinx-theme.css?digest=fb52004a9691e5b75515" rel="stylesheet">

    
  <link rel="stylesheet"
    href="_static/vendor/fontawesome/5.13.0/css/all.min.css">
  <link rel="preload" as="font" type="font/woff2" crossorigin
    href="_static/vendor/fontawesome/5.13.0/webfonts/fa-solid-900.woff2">
  <link rel="preload" as="font" type="font/woff2" crossorigin
    href="_static/vendor/fontawesome/5.13.0/webfonts/fa-brands-400.woff2">

    <link rel="stylesheet" type="text/css" href="_static/pygments.css" />
    <link rel="stylesheet" type="text/css" href="_static/graphviz.css" />
    <link rel="stylesheet" type="text/css" href="_static/../_font/fontawesome/css/all.min.css" />
    <link rel="stylesheet" type="text/css" href="_static/../_font/fontawesome/css/all.min.css" />
    <link rel="stylesheet" type="text/css" href="_static/design-style.b7bb847fb20b106c3d81b95245e65545.min.css" />

Copy link
Collaborator

@choldgraf choldgraf May 11, 2022

Choose a reason for hiding this comment

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

hmmm yeah this feels related to @pradyunsg's concerns about !important in that library. @pradyunsg was there any way you got around this other than just !importanting everything?

Copy link
Contributor

Choose a reason for hiding this comment

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

When two selectors have the same "specificity", then the latter definition is used. That's the situation here. You can work around this without !importants here by having a more specific selector -- like body .sd-card (that increases the specificity by 1, which might just be enough). :)

Most of the conflicting styles here aren't !important'd, so it's not a big deal. The issues I had with !important in that library are around this file: https://github.com/executablebooks/sphinx-design/blob/cca2cfb1b4c0a55ecf6661889c52ea320d42f58f/style/_display.scss

Furo has a separate furo-extensions.css file that is loaded after the extensions, to deal with some of this -- but realistically, unless you're overriding !importants (you're not, AFAICT), you can typically get away with a more specific selector. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ooh that is a great point. Can we try increasing the specificity just a bit to see if we can add these fixes without the "important" lines?

warnings.txt Outdated Show resolved Hide resolved
12rambau and others added 6 commits May 11, 2022 14:09
Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
@choldgraf
Copy link
Collaborator

I pushed a few minor changes to the docs:

  • Moved the two sphinx-panels admonitions into a single one, so it's easier to read.
  • Added an example of over-riding the sphinx-design primary color

I think the only thing remaining here is to see if we could remove some of the !important lines by making the CSS selectors more specific. Is that right @12rambau ?

@12rambau
Copy link
Collaborator Author

yep. I'm waiting for the ng-sphinx PR to be merged to use the new classes in the css files

@12rambau
Copy link
Collaborator Author

@choldgraf ready for review, no more !important to be seen.

Copy link
Collaborator

@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.

Nice! This looks good to me, very happy there's no more !important!

@choldgraf choldgraf changed the title add panels to the docs Add panels support May 24, 2022
@choldgraf choldgraf merged commit 734c99f into pydata:master May 24, 2022
@12rambau 12rambau deleted the panel branch May 24, 2022 08:35
@jarrodmillman jarrodmillman added this to the 0.9 milestone May 25, 2022
@damianavila
Copy link
Collaborator

Is there a follow-up issue to get rid of sphinx-panels? I have not seen one yet.

@choldgraf
Copy link
Collaborator

I don't think so, but agree we should deprecate it and switch to Sphinx design!

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.

BUG: CSS in dark mode for cards
5 participants