-
Notifications
You must be signed in to change notification settings - Fork 39
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
Conversation
The purple color comes from: Looks like we don't set that one in the theme... |
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", |
There was a problem hiding this comment.
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.
"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...?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
"announcement": <URL>
in our conf.py to make an announcement.- 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍🏻
This reverts commit 0096543.
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. |
Holy cow is that different! Here's chrome: Edit: any chance you have some sort of dark-mode things enabled that could also be playing a role? |
Yup this seems to be the color set for the dark mode in the pydata sphinx theme: We probably want to set that up in https://github.com/napari/docs/blob/main/docs/_static/custom.css |
I think you're right, @psobolewskiPhD and @melissawm ! Here it is when switching to "light mode". |
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. |
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? |
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. |
Ok, thanks for the clear explanation. |
html_theme_options section of conf.py using the key: "announcement" | ||
--> | ||
<div class="sidebar-message"> | ||
This is a community-supported project. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets try the link
OK, I updated the OP, but the general plan, as discussed here:
|
# 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)).
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. 😅 |
# 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>
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