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

Bump napari sphinx theme version #423

Merged
merged 7 commits into from
Jun 1, 2024
Merged

Bump napari sphinx theme version #423

merged 7 commits into from
Jun 1, 2024

Conversation

melissawm
Copy link
Member

References and relevant issues

Depends on napari/napari-sphinx-theme#161

(and a new release of the theme)

Description

Restores the sidebar after the PyData Sphinx Theme is updated to version 0.15.3

@github-actions github-actions bot added the documentation Improvements or additions to documentation label May 29, 2024
@melissawm melissawm added maintenance CI, dependencies, and other maintenance and removed documentation Improvements or additions to documentation labels May 29, 2024
@psobolewskiPhD psobolewskiPhD added this to the 0.5.0 milestone May 30, 2024
@github-actions github-actions bot added the documentation Improvements or additions to documentation label May 30, 2024
@psobolewskiPhD
Copy link
Member

I reran CI, I think.

@psobolewskiPhD
Copy link
Member

Interesting, I can't re-run the circleCI--it's run from your fork!

@jni
Copy link
Member

jni commented May 30, 2024

Maybe too soon? The rendered docs on Circle-CI and the artifact both show a back-to-top button.

@jni
Copy link
Member

jni commented May 30, 2024

We can run by pushing an empty commit

@jni
Copy link
Member

jni commented May 30, 2024

should I do that?

@jni
Copy link
Member

jni commented May 31, 2024

I did the thing

@psobolewskiPhD
Copy link
Member

Hmm, i downloaded the artifact, but it doesn't appear to be using the right version. It's on pypi.

@psobolewskiPhD
Copy link
Member

Got it -- constraints. Need to go PR napari/napari

@jni
Copy link
Member

jni commented May 31, 2024

gdi 😂

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 31, 2024

Yeah, always forget. Wish there was a way to push that from here. Like could the bot make a PR to napari if we trigger it from here?
Anyhow, hope CI passes
napari/napari#6947

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 31, 2024

I think the pip freeze step I added is useful? so maybe we just leave it? Or do you want me to revert it

https://github.com/napari/docs/actions/runs/9310906726/job/25629186054?pr=423#step:9:113

@psobolewskiPhD
Copy link
Member

Rerunning GitHub CI (can't do anything about circle)

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 31, 2024

It worked, here is circle:
https://output.circle-artifacts.com/output/job/9c07bed9-f15a-48a3-9b63-94a2a46023c3/artifacts/0/docs/docs/_build/html/index.html

Only thing I notice is the very low contrast on the 2nd tab here (plugins page)
image
Same on the installation page:
image
Not sure if that is set in theme or here in docs.

@melissawm
Copy link
Member Author

Oh I missed that! Its the theme. I'll fix

@melissawm
Copy link
Member Author

Ooohhh I see why I missed it - it's one of those dark theme things. For me, using a light theme, this is what I see:

Captura de imagem_20240531_102856

I'll send a PR to the theme, but if we don't want to do a full new release for it to take effect I can add a workaround commit here.

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 31, 2024

@melissawm Wait, doesn't that mean that #352
will fix it?
By pinning light theme?

And yup, here is the screenshot during the day:
image

@psobolewskiPhD
Copy link
Member

Hmm. If we merge #352 the dev will rebuild with the new theme right, because the pins and constraints are lifted, so until this is merged there will be the issues in dev.
Maybe we should cherry pick #352 into this PR and then we can see if it's resolved?

@melissawm
Copy link
Member Author

Oh maybe it will, sorry I forgot about #352! Let me do the cherry-picking and see if it does.

@melissawm
Copy link
Member Author

For the tabs specifically, it doesn't - but I think I know how to fix it. Will update here soon

@melissawm
Copy link
Member Author

I have no idea what's going on in CI... but locally this works

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 31, 2024

Alas, #352 (comment) doesn't fix it.
Even though as far as I can tell it's implemented correctly:
https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/light-dark.html#configure-default-theme-mode

It's still switching rather than staying light. maybe this is still bugged upstream?

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 31, 2024

Yup, your patch works as a bandaid, but we're still getting theme switching.
Here's when I switch system to dark:
image

Note the tabs are not grey anymore.
Obviously, not a blocker. But I wish I could understand why we can't properly pin to using Light theme exclusively.

Edit: also note that now all the signaling is done by the outline box, rather than the dimming of the text as previously.

@melissawm
Copy link
Member Author

Ok something is very weird. This is what I see in CircleCI:

Captura de imagem_20240531_120830

Note I'm selecting the dark mode from the inspect tab...

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 31, 2024

@melissawm I just checked and the numpy docs use the "default_mode": "light" directive
https://github.com/numpy/numpy/blob/22dc07c0b3298abc2b55a045b6842c7fff899bbb/doc/source/conf.py#L275
and it works there!
But, when I click their pydata-sphinx-theme.js in the console it's a different script than what we have:

function r(t) {
        "light" !== t && "dark" !== t && "auto" !== t && (console.error(`Got invalid theme mode: ${t}. Resetting to auto.`), t = "auto");
        var e = i.matches ? "dark" : "light";
        document.documentElement.dataset.mode = t;
        var n = "auto" == t ? e : t;
        document.documentElement.dataset.theme = n,
        localStorage.setItem("mode", t),
        localStorage.setItem("theme", n),
        console.log(`Changed to ${t} mode using the ${n} theme.`),
        i.onchange = "auto" == t ? o : ""
    }

So I guess a different version of the pydata-sphinx. So I assume a regression happened somewhere, but maybe we can figure it out?
I don't have any more time on this right now but I will try to poke around later or over the weekend.

@melissawm
Copy link
Member Author

Also - the general design of these tabs is a new accessibility-focused design: pydata/pydata-sphinx-theme#1838

I didn't alter it at all except for the colors.

@psobolewskiPhD
Copy link
Member

Right, the tabs are just the symptom of the theme changing.
We can whack a mole with fixing any CSS manually or we can figure out how to prevent the switching or we can develop a full dark theme.

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented May 31, 2024

Gah I checked numpy dev docs which look like 15.3 and it the theme doesn't switch there either!
And the js is definitely newer:

function setTheme(mode) {
  if (mode !== "light" && mode !== "dark" && mode !== "auto") {
    console.error(`Got invalid theme mode: ${mode}. Resetting to auto.`);
    mode = "auto";
  }

  // get the theme
  var colorScheme = prefersDark.matches ? "dark" : "light";
  document.documentElement.dataset.mode = mode;
  var theme = mode == "auto" ? colorScheme : mode;
  document.documentElement.dataset.theme = theme;
  // TODO: remove this line after Bootstrap upgrade
  // v5.3 has a colors mode: https://getbootstrap.com/docs/5.3/customize/color-modes/
  document.querySelectorAll(".dropdown-menu").forEach((el) => {
    if (theme === "dark") {
      el.classList.add("dropdown-menu-dark");
    } else {
      el.classList.remove("dropdown-menu-dark");
    }
  });

  // save mode and theme
  localStorage.setItem("mode", mode);
  localStorage.setItem("theme", theme);
  console.log(`[PST]: Changed to ${mode} mode using the ${theme} theme.`);

  // add a listener if set on auto
  prefersDark.onchange = mode == "auto" ? autoTheme : "";
}

Do we need to try to set the"default_mode": "light" in napari-sphinx-theme somehow instead of here?

Edit: the new numpy docs are beautiful:
https://numpy.org/devdocs/building/index.html
😍

@melissawm
Copy link
Member Author

melissawm commented May 31, 2024

@psobolewskiPhD so I think I need to understand what the problem is here.

  1. The tab design is inherited from the pydata sphinx theme, and I manually set the colors to work exactly the same in light and dark mode, independent of which is selected. So me and you should see the same thing in CI. If we don't, this may be a browser issue? I'm on firefox, and if I remember well you are on Safari - could it be something's happening there?
  2. A couple of days ago I updated the NumPy docs to use the current PyData Sphinx Theme, so I don't understand why we would have different javascripts. MAINT: Unpin pydata-sphinx-theme numpy/numpy#26568

I think we have a couple moving parts here:

  • Merging this PR and solving the immediate issue of low-contrast text in the tabs
  • Try to debug the dark/light theme switching and publish a new version of the theme when that's done

These seem independent to me, so we could merge this one if it's good enough (since it also contains fixes for the sidebar toc and other stuff) and keep debugging on #352?

@psobolewskiPhD
Copy link
Member

@melissawm
OK I think I have everything figured out. I posted in the details in the theme PR (#352 (comment)), but the gist is that the theme mode setting is cached in a cookie which overrides the default setting. So I had auto from before (the old default setting) and it overrides the new default_mode light.
So the theme was still switching between light and dark automatically with my system.
Purging cookies or using incognito solved it. A bit annoying.
There could also be browser based settings at play, like accepting cookies, timers of cookie, or support for auto switching.

Anyhow, I think your bandaid for the tabs works and covers anyone that got an auto cookie till the cookie expires.
I think we should be good to go then.

@jni
Copy link
Member

jni commented Jun 1, 2024

Thanks for the thorough investigation! ❤️‍🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation maintenance CI, dependencies, and other maintenance
Projects
None yet
3 participants