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

Issue when interacting with furo's light theme #53

Closed
OriolAbril opened this issue Jan 28, 2022 · 10 comments
Closed

Issue when interacting with furo's light theme #53

OriolAbril opened this issue Jan 28, 2022 · 10 comments
Labels
bug Something isn't working

Comments

@OriolAbril
Copy link
Contributor

Describe the bug

When I do create only-dark grid items they are not hidden when switching to the light theme. This seems to be because sd-d-flex-row takes precedence over only-dark. The same doesn't happen with only-light as in this case it's only-light that takes precedence over d-flex-row. It doesn't happen either if I set the only- classes to the whole grid.

I expected grid items to be displayed/hidden like other items with the only- classes.

I am trying to set up an about website for ArviZ: https://arviz-governance--3.org.readthedocs.build/en/3/, and wanted to use a grid with grid-item-card to display the logos of the sponsors as clickable links, and some need different links between dark and light theme while others don't.

Is this a bug or something that could be fixed? Am I attempting something that is not supported? I am mostly puzzled as to why the behaviour depends on light/dark, not knowing web things I'd expect symmetry, both working or both failing 😅

cc @pradyunsg

Note: I can set the whole grid to be show/hidden instead, but as only some sponsors need the change, it would mean extra duplication.

Reproduce the bug

You can see it in the PR preview: https://arviz-governance--3.org.readthedocs.build/en/3/#sponsors-and-institutional-partners

List your environment

No response

@OriolAbril OriolAbril added the bug Something isn't working label Jan 28, 2022
@pradyunsg
Copy link
Member

Hmmm... interesting! I think this is something should can be treated as a bug in sphinx-design, but I'm not a 100% sure.

I'm wondering why .sd-d-flex-row needs !important in its declarations. What's the rationale there? If you want high specificity, you can get that via putting this under body [role=main] div.sd-d-flex-row or something similar?

@chrisjsewell
Copy link
Member

Heya,

I'm wondering why .sd-d-flex-row needs !important in its declarations.

it comes directly from bootstrap

Not saying that I don't want to fix it 😄, but these only-light / only-dark are furo specific classes yes? these !important have not been a problem before, is there a discrepancy in the furo css, as to why they would act differently?

@pradyunsg
Copy link
Member

pradyunsg commented Jan 28, 2022

They’re Furo specific since there’s exactly one theme in the ecosystem with dark mode. ;)

There’s no “discrepancy” and the reason it’s a problem here is because these extension-specific !important styles are overriding the theme’s mechanism to have content only show up in light/dark mode (only-dark and only-light).

Again, is there a specific reason that these display classes have !important on them, beyond “just copied bootstrap”? If not, I encourage removing them because I’m pretty sure no one is setting styles for sd-d-flex-row and friends accidentally. :)

@OriolAbril
Copy link
Contributor Author

If that is an option and @chrisjsewell agrees I can test it and send a PR removing those !important if it works.

@chrisjsewell
Copy link
Member

If that is an option and @chrisjsewell agrees I can test it and send a PR removing those !important if it works.

In principle, yeh I don't object to it. Although obviously I would want to check very thoroughly, that it does not inadvertently cause other issues, with furo or any other theme

@OriolAbril
Copy link
Contributor Author

OriolAbril commented Feb 8, 2022

I have removed the importants from the display.scss file and tested out the result. The light/dark change on grid items only now works, but the light/dark only elements have a different alignment behaviour, as seen in https://oriol-sphinx-playground--2.org.readthedocs.build/en/2/.

Note: I still haven't opened a PR here with the !important deletions as I'm still not sure if the fix will work, but have no problem doing so if it helps you review what is happening.

@pradyunsg
Copy link
Member

Did you modify the original reproducer for this issue? I'm not able to reproduce this on that link anymore. :)

@pradyunsg
Copy link
Member

Okay, I've spent a bit more time investigating this and this might actually be feasible with a Furo-side change to ensure that it doesn't set display: block in light/dark mode, and to ensure that it has a selector more specific (hopefully?) than sphinx-design for hiding things.

Can you try with Furo's main branch if you can still reproduce this issue? If not, let's close this as solved via a Furo-specific solution. :)

pip install https://github.com/pradyunsg/furo/archive/refs/heads/main.zip to install Furo's main branch in a Python environment.

@OriolAbril
Copy link
Contributor Author

Did you modify the original reproducer for this issue? I'm not able to reproduce this on that link anymore. :)

Yes, sorry. I want to get that original PR merged next week and I need also reviews from the university. I have for now fixed the issue by adding the only-dark/light to the whole grid instead of doing that on grid items, which works but requires duplicating all the logos that don't change between modes.

That is why I have shared a 2nd link which is also much smaller repo showing only the grid with the issue.

I will try changing the dependencies. I'll set latest sphinx-design release plus furo@main and see how it goes. Thanks!

@pradyunsg
Copy link
Member

Cool, https://pradyunsg.me/furo/kitchen-sink/sphinx-design/#grid-cards has the sanity check for this. It works, with changes only on Furo's end. The fix will be in the next Furo release, which I'll cut over the weekend. :)

Screen.Recording.2022-02-10.at.18.32.07.mov

Thanks @chrisjsewell for the pushback here -- I think having this handled the way that's it's handled in Furo now is better and cleaner overall than what I was suggesting earlier! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants