napari / docs

Documentation for napari (other than API docs)
BSD 3-Clause "New" or "Revised" License
9 stars 37 forks source link

Bump napari sphinx theme version #423

Closed melissawm closed 1 month ago

melissawm commented 1 month ago

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

psobolewskiPhD commented 1 month ago

I reran CI, I think.

psobolewskiPhD commented 1 month ago

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

jni commented 1 month ago

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

jni commented 1 month ago

We can run by pushing an empty commit

jni commented 1 month ago

should I do that?

jni commented 1 month ago

I did the thing

psobolewskiPhD commented 1 month ago

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

psobolewskiPhD commented 1 month ago

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

jni commented 1 month ago

gdi 😂

psobolewskiPhD commented 1 month ago

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 https://github.com/napari/napari/pull/6947

psobolewskiPhD commented 1 month ago

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 commented 1 month ago

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

psobolewskiPhD commented 1 month ago

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 commented 1 month ago

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

melissawm commented 1 month ago

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 commented 1 month ago

@melissawm Wait, doesn't that mean that https://github.com/napari/docs/pull/352 will fix it? By pinning light theme?

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

psobolewskiPhD commented 1 month ago

Hmm. If we merge https://github.com/napari/docs/pull/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 https://github.com/napari/docs/pull/352 into this PR and then we can see if it's resolved?

melissawm commented 1 month ago

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

melissawm commented 1 month ago

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

melissawm commented 1 month ago

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

psobolewskiPhD commented 1 month ago

Alas, https://github.com/napari/docs/pull/352#issuecomment-2142291514 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?

melissawm commented 1 month ago

Here we go: https://output.circle-artifacts.com/output/job/637b4214-aea5-42cd-a028-027653465db8/artifacts/0/docs/docs/_build/html/tutorials/fundamentals/installation.html

psobolewskiPhD commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

Also - the general design of these tabs is a new accessibility-focused design: https://github.com/pydata/pydata-sphinx-theme/discussions/1838

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

psobolewskiPhD commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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. https://github.com/numpy/numpy/pull/26568

I think we have a couple moving parts here:

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 commented 1 month ago

@melissawm OK I think I have everything figured out. I posted in the details in the theme PR (https://github.com/napari/docs/pull/352#issuecomment-2143042137), 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 commented 1 month ago

Thanks for the thorough investigation! ❤️‍🔥