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

NEW: Adding announcement banners #722

Merged
merged 8 commits into from
Jun 23, 2022
Merged

NEW: Adding announcement banners #722

merged 8 commits into from
Jun 23, 2022

Conversation

choldgraf
Copy link
Collaborator

This adds functionality for announcement banners. This lets theme authors define some text that shows up at the top of the page, and disappears as somebody scrolls down. It uses the same CSS hack we use for admonitions in order to color the background by the primary color. It also cleans up some CSS rules that makes our CSS a bit less hacky :-)

Here's how it looks with a little demo:

chrome_wpK3h1IHX2

I didn't add a test for this mostly because the functionality is purely in the templates and there isn't really much extra functionality to text (the HTML shows up or it doesn't, that's it), but I'm happy to add a test if folks think it'd be helpful.

@dopplershift
Copy link

So if I'm reading this right, the only way to add/change an announcement is to rebuild the docs?

The functionality I'd love to have built-in is to add/remove the announcement by editing a file in the base of the docs repo. That way even old/stable versions get the announcement. Not sure if that's compatible with your vision for it, though.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 12, 2022

One way to do what you describe would be to just follow that workflow yourself, and read in the text file via conf.py then insert it into that keyword value. You could then add a step that downloads the latest version of the text file at build time.

Though, you'd still need to rebuild the docs. I guess the only way to do it without rebuilding docs would be to do something like the version drop-down, that used JavaScript to read a text file at page load time. I'm not really sure how to do that in a nice way w the CSS though...maybe it could be a follow up PR

@dopplershift
Copy link

Sure. So what I have right now has extended the theme to put in an empty div and then have javascript that looks for a non-empty /banner.html and populates the div with that. I'm not sure what the right way to handle that in the theme would be.

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

I really like this feature. Nothing to add.

@choldgraf
Copy link
Collaborator Author

Thinking about @dopplershift's request, what if we supported something like this:

  • If announcement starts with http, then treat it as the source of external HTML that should be used to populate the announcement bar.
  • If so, then add some JavaScript that makes an HTTP request for the value of announcement, and places whatever it finds inside the announcement banner.
  • If not, then assume announcement is itself some HTML, and simply insert it into the template.

@choldgraf
Copy link
Collaborator Author

I think that the latest commit will add this functionality. It works kinda like the version switcher now, though only if announcement starts with http

@dopplershift
Copy link

That sounds great to me! Thanks, this is one less thing we have to tweak the theme to add.

@choldgraf
Copy link
Collaborator Author

@12rambau wanna take another quick look to make sure you're +1 on the URL-based approach this now supports as well?

You can add an announcement banner that draws extra attention from your reader.
It will be displayed at the top of the screen, but will disappear once you start scrolling.

To add an announcement banner, use the ``html_theme_options.announcement`` configuration.
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't these be html_theme_options['announcement'] (rather than the dot syntax)? It is a python dictionary after all.

docs/user_guide/configuring.rst Outdated Show resolved Hide resolved
{%- if is_remote %}
<script>
$.get("{{ theme_announcement }}", success=(data)=>{
console.log(data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: is this considered JS best practice (to console.log on successful get)? Or is this cruft left over from the process of developing this feature? (I honestly don't know, but my instinct would be to only log the failures)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is cruft!

Copy link
Collaborator

Choose a reason for hiding this comment

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

if that's the case I need to edit the theme code as well I think I'm consol.log every time the mode is changed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that it's fine to do this when the theme mode is changed, but in this case it was cruft haha

docs/conf.py Outdated
@@ -108,6 +108,8 @@
},
"use_edit_page_button": True,
"show_toc_level": 1,
# "announcement": "Here's a test <a href='https://google.com'>announcement</a>!",
"announcement": "https://raw.githubusercontent.com/choldgraf/pydata-sphinx-theme/header/docs/_templates/custom-template.html",
Copy link
Collaborator

Choose a reason for hiding this comment

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

should choldgraf change to pydata here before merging?

docs/conf.py Outdated Show resolved Hide resolved
Co-authored-by: Daniel McCloy <dan@mccloy.info>
@choldgraf
Copy link
Collaborator Author

I believe I've addressed @drammock's ideas!

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

two more :)

docs/user_guide/configuring.rst Outdated Show resolved Hide resolved
docs/user_guide/configuring.rst Outdated Show resolved Hide resolved
choldgraf and others added 2 commits June 16, 2022 07:42
docs/conf.py Outdated Show resolved Hide resolved
@choldgraf
Copy link
Collaborator Author

OK I think this one is ready to go - anybody want to hold off or shall we give it a whirl? :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants