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 placeholder announcement and html file for announcements #332

Closed
wants to merge 14 commits into from

Conversation

psobolewskiPhD
Copy link
Member

@psobolewskiPhD psobolewskiPhD commented Jan 18, 2024

References and relevant issues

Additional relevant discussion here:
https://napari.zulipchat.com/#narrow/stream/215289-release/topic/0.2E4.2E19/near/418837322

Description

This PR uses one of the new features of the new theme: it adds a placeholder announcement (text string) in conf.py plus the html file that can be used to change the announcement without release/rebuilding docs.
See https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/announcements.html
(and also pydata/pydata-sphinx-theme#722 (comment) for some nitty gritty)
The most important part is that to change the text of the announcement without release/rebuilding docs, you need to use a http link in conf.py
I added comments to the files to explain the process.
After this PR is merged to add the html file, we will need a followup PR with the change to conf.py to uncomment the http link to use the html file. Then in the future, this html file can be edited to change the announcement banner text on the fly, e.g. for the Survey

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 18, 2024
docs/conf.py Outdated Show resolved Hide resolved
@psobolewskiPhD
Copy link
Member Author

psobolewskiPhD commented Jan 18, 2024

The purple color comes from:
https://github.com/pydata/pydata-sphinx-theme/blob/bd0eacc00fdf3d143727479f9183c23761c52ab7/src/pydata_sphinx_theme/assets/styles/sections/_announcement.scss#L42-L47

Looks like we don't set that one in the theme...
I personally don't dislike it, we have purple in the napari logo, and it does draw attention.

@jni
Copy link
Member

jni commented Jan 19, 2024

Yes I like the purple! Let's keep it! 😃

docs/conf.py Outdated
@@ -110,6 +110,7 @@
"secondary_sidebar_items": ["page-toc"],
"pygment_light_style": "napari",
"pygment_dark_style": "napari",
"announcement": "https://raw.githubusercontent.com/psobolewskiPhD/napari-docs/announcement/docs/_templates/custom-template.html",
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this should be a local path, not a link to a GH blob? e.g.

Suggested change
"announcement": "https://raw.githubusercontent.com/psobolewskiPhD/napari-docs/announcement/docs/_templates/custom-template.html",
"announcement": "_templates/custom-template.html",

? Does that work?

Also I would call the file something else since it's a full announcement, not a template?

The class name (sidebar message) is somewhat misleading too, but I guess we just inherit that...?

Copy link
Member Author

Choose a reason for hiding this comment

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

It has to be accessible via http to make it work, so I think that won't work.
I think the name of the html file doesn't matter, but re-used everything from pydata-sphinx-theme docs as a first try.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I just found the docs for this (I might consider adding a link to the conf.py as a comment above the "announcement" key). Since you're not using any fancy html, I would suggest we mostly use the text directly?

https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/announcements.html#provide-local-html-in-your-theme

Alternatively, if we want to keep all old announcements around for reference, we could have a folder full of announcements, e.g. https://napari.org/dev/_static/announcements/survey-2023.html, and just point to those. (I guess then you want to first make a PR for the announcement html and then make one for adding it to the banner.)

Copy link
Member Author

@psobolewskiPhD psobolewskiPhD Jan 19, 2024

Choose a reason for hiding this comment

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

I'll definitely add comments to conf.py once we have it sorted.
Regarding just using text, my understanding is that will only add the announcement on the dev site, based on:
https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/announcements.html#insert-remote-html-with-javascript
and the conversation starting here:
pydata/pydata-sphinx-theme#722 (comment)

To get it to work on the other sites, without rebuilding, it has to be an http link to a file.

Do we need historic announcements in the deployed site? there will be history in our git repo right?
Also do we want to always show some sort of (generic) welcome announcement, ala the pydata-sphinx-theme docs?
I'd vote no, because then real announcements stand out more...

So I think I'd vote to just having a fixed location for the announcement file and then comment/uncomment in conf.py when we want it to go live in a PR that edits the copy.

In this PR I'd just launch a generic annoucement so we can see it all works right before the survey, news, etc. to set up the machinery and then followup PRs with real announcements.
Can add a PR to the contributing docs re: how to make announcements.

Copy link
Member

Choose a reason for hiding this comment

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

Wow — even reading it now it takes a while for me to parse that that is indeed the consequence of the html vs text announcement. Good job grokking this @psobolewskiPhD! 😅

I agree — no default announcement.

But:

So I think I'd vote to just having a fixed location for the announcement file and then comment/uncomment in conf.py when we want it to go live in a PR that edits the copy.

I think the opposite here: announcement URL is fixed and uncommented, and indeed that needs to be cherry-picked urgently for 0.4.19 so that it will pick up announcements. Then making or removing announcements amounts only to adding or removing an html file in a specific location. My proposal is something like https://napari.org/dev/_static/announcements/announcement.html. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I totally follow?
As far as I can tell what we need is:

  1. "announcement": <URL> in our conf.py to make an announcement.
  2. a accessible via http page with the announcement content.

So for 1 above it sounds like you're suggesting we always have the conf.py line pointing to that URL, but sometimes the file is not there? This will result in a 404 banner -- I already tested this by accident. To not have an announcement banner you need to delete/comment-out the "announcement:" part of conf.py
Maybe one could edit the file to exist but be empty? Let me try in this PR.

For 2 above the URL you suggest should work, fine by me. I'll change the path in this PR.

And yeah, I think we need the new theme and this PR cherry picked to 0.4.19 for this to work on napari.org (because it points to stable, which isn't rebuilt), although I can't actually figure out where/how the docs release happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, no, that URL won't work right now because that file isn't deployed so it's not there when the conf.py is used at docs build. chicken and egg problem.
I suspect this is why pydata people just point to the GitHub source.

So if we did want that, we'd need a PR that adds that file, merge and deploy it so it's there on napari.org, then PR back here to set/change the link in conf.py

Copy link
Member Author

Choose a reason for hiding this comment

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

And again just to clarify (maybe?), my understanding is that with the http location approach you can change the announcement by changing the file without rebuilding docs--but they still need to be built with the conf.py flag enabled (and the proper theme version etc.)
So if we want this on stable soon, we need to push this plus the theme PR into 0.4.19 release docs.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is the same as yours @psobolewskiPhD - I think that plan is good 👍🏻

@psobolewskiPhD
Copy link
Member Author

psobolewskiPhD commented Jan 20, 2024

I pushed a commit with that announcement source html file empty.
If we don't want a permanent, default announcement then it would be ideal for us if the empty file made an empty announcement that was basically invisible.
Then when we want an announcement, we edit the file and boom it JustWorks.

Edit: Alas empty file yields an empty Announcement
image

So it looks like at present showing/removing Announcements on napari.org would require editing conf.py which would require a release.
But editing the content of the announcement would work without a release by editing the source of the html file.

@kevinyamauchi
Copy link
Contributor

Thanks for working on this @psobolewskiPhD ! I like the purple in this screenshot. However, when i tried viewing the "rendered docs" on the CI checks, the purple was much darker, making it hard to read the text. In case it matters, I'm on Mac OS + Firefox.

I'm not sure if you're just testing, but I saw the message in zulip, so I thought I'd flag it just in case.

Screenshot 2024-01-23 at 19 36 10

@psobolewskiPhD
Copy link
Member Author

psobolewskiPhD commented Jan 23, 2024

Holy cow is that different! Here's chrome:
image
@kevinyamauchi can you check what the announcement looks like here:
https://pydata-sphinx-theme.readthedocs.io/en/stable/

Edit: any chance you have some sort of dark-mode things enabled that could also be playing a role?

@melissawm
Copy link
Member

Yup this seems to be the color set for the dark mode in the pydata sphinx theme:
Screenshot 2024-01-23 at 5 55 32 PM

We probably want to set that up in https://github.com/napari/docs/blob/main/docs/_static/custom.css

@kevinyamauchi
Copy link
Contributor

I think you're right, @psobolewskiPhD and @melissawm ! Here it is when switching to "light mode".

Screenshot 2024-01-24 at 08 56 10

@melissawm
Copy link
Member

I think there are two options, either setting that in the custom.css file I pointed above or fixing it in the theme proper. Second one would be better long term but the first one doesn't require a new release of the theme. In any case, we probably want to check for a designer who could help us with the dark theme, as currently it is just the same as the light theme.

@psobolewskiPhD do you want to include the fix in this pr or a new one? I can push the fix to this pr if it's easier for you.

@psobolewskiPhD
Copy link
Member Author

I'm still not sure where the issue lies: our theme doesn't advertise a light or a dark mode, so isn't this just Firefox doing theming?
I have an extension that does that and sometimes it too gives odd results, but it's out of our control barring a real dark and light theme, right?

@melissawm
Copy link
Member

Actually no. The way the PyData Sphinx Theme is configured, it has a light and dark mode pre-defined. It can be set on the website with a button, but it does actually also pick up stuff from your system. Because I had to do some heavy customization to make it look like napari colors, I overrode some of the colors, but as you can see here not all :) So I think we might as well fix this to avoid other issues coming up.

This will mean we explicitly say to browsers "use this color even if the system is using a dark theme". In the future, if we want to set up a proper dark theme, we can reconsider.

@psobolewskiPhD
Copy link
Member Author

Ok, thanks for the clear explanation.
Any thoughts on the file location stuff?
My vote would be to just do what the pydata.docs does.
And given that the theme is already cherry picked maybe we make this PR include the survey already? And then if we want to stop the survey we just change the html to a more generic message and in 0.5.0 we can remove the announcement entirely--or use it to advert the new version (or the blog news whatever)

@psobolewskiPhD psobolewskiPhD changed the title Test adding announcement Add placeholder announcement and html file for announcements Jan 30, 2024
html_theme_options section of conf.py using the key: "announcement"
-->
<div class="sidebar-message">
This is a community-supported project.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add the survey text and link already?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, I was sort of thinking of making this a placeholder and then making a survey PR just for better history

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah thinking more, I think it's better we have a default/placeholder announcement and then we PR the survey one as needed--and we have something to go back to.

Copy link
Member

Choose a reason for hiding this comment

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

ok sounds good!

docs/conf.py Outdated
@@ -110,6 +110,10 @@
"secondary_sidebar_items": ["page-toc"],
"pygment_light_style": "napari",
"pygment_dark_style": "napari",
# hard coded announcement for banner at the top:
"announcement": "This is a community-supported project! There's many ways to contribute—click Contributing below for info.",
Copy link
Member

Choose a reason for hiding this comment

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

And here as well, we could add the survey text and link.

Copy link
Member Author

Choose a reason for hiding this comment

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

As noted above, I think it's best to just have a placeholder announcement here and then we PR the survey as needed. The only question is where to keep this one plain text like I did or put in a link to dev/contributing docs -- sneaky way around the versioned docs!

Copy link
Member Author

Choose a reason for hiding this comment

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

lets try the link

@psobolewskiPhD psobolewskiPhD added the maintenance CI, dependencies, and other maintenance label Jan 30, 2024
@psobolewskiPhD
Copy link
Member Author

OK, I updated the OP, but the general plan, as discussed here:
https://napari.zulipchat.com/#narrow/stream/215289-release/topic/0.2E4.2E19/near/418837322
is that:

  1. this PR uses a text placeholder announcement and places the html file in the repo
  2. followup PR activates using the html file by using the http option in conf.py
    That 2nd PR would need to be cherry picked for this to go live on stable 0.4.19 docs.
    cc: @melissawm @jni @Czaki

@psobolewskiPhD psobolewskiPhD added this to the 0.5.0 milestone Jan 30, 2024
@psobolewskiPhD
Copy link
Member Author

Everything working with the text announcement:
image

I will change it now to use a link to dev contributing guide -- sneaky work around until the Contributing guide always points to latest.

@psobolewskiPhD
Copy link
Member Author

Ok, I think everything is gucci now:
image

I updated the OP.

jni added a commit that referenced this pull request Jan 31, 2024
# References and relevant issues

First part of #332: make sure the banner html is publicly accessible on
napari.org.

# Description

Discussion [on
Zulip](https://napari.zulipchat.com/#narrow/stream/215289-release/topic/0.2E4.2E19/near/418942751).

After this is merged, the banner text will live in

https://napari.org/dev/_static/announcement.html

forever. Since that file can be updated with a merge to main here, it is
easy
to add or remove announcements, pending the pydata-sphinx-theme removing
the
banner when the text is empty (see @melissawm's [changes soon to be

suggested](melissawm/pydata-sphinx-theme@master...empty-announcement)).
@jni
Copy link
Member

jni commented Jan 31, 2024

Superseded by #341 and #342 — but thank you @psobolewskiPhD and @melissawm for blazing the path! 🗺️ I'm going to close this so it doesn't get merged accidentally. 😅

@jni jni closed this Jan 31, 2024
Czaki added a commit that referenced this pull request Jan 31, 2024
# References and relevant issues

Together with #341, this PR supersedes #332.

# Description

The announcement banner text html was added in #341. This PR now points
our
config to it, allowing the banner to be displayed.

Note: #332 also has a CSS fix. However, that fix is included in the
latest
release of the napari-sphinx-theme, so I think it's best to leave it out
of
this PR. (I would be happy to make a separate PR for it if we don't want
to
wait until the dependencies are updated.)

Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
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
Development

Successfully merging this pull request may close these issues.

4 participants