Skip to content

Fix display of local announcement banner #1822

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix display of local announcement banner #1822

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 28 additions & 23 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/sections/announcement.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,9 @@
{% set header_classes = ["bd-header-announcement", "container-fluid", "init"] %}
{% set is_remote=theme_announcement.startswith("http") %}
{# If we are remote, add a script to make an HTTP request for the value on page load #}
{%- if is_remote %}
<script>
document.write(`<aside class="bd-header-announcement d-print-none" aria-label="{{ banner_label }}"></aside>`);
fetch("{{ theme_announcement }}")
.then(res => {return res.text();})
.then(data => {
if (data.length === 0) {
console.log("[PST]: Empty announcement at: {{ theme_announcement }}");
return;
}
div = document.querySelector(".bd-header-announcement");
div.classList.add(...{{ header_classes | tojson }});
div.innerHTML = `<div class="bd-header-announcement__content">${data}</div>`;
function setAnnouncementHeight() {
const div = document.querySelector(".bd-header-announcement");
// At least 3rem height
const autoHeight = Math.min(
div.offsetHeight,
Expand All @@ -36,15 +26,30 @@
div.style.setProperty("min-height", "3rem");
}, 320);
}, 10);
})
.catch(error => {
console.log("[PST]: Failed to load announcement at: {{ theme_announcement }}");
});
}
is_remote = {{ is_remote | tojson }};
if (is_remote) {
document.write(`<aside class="bd-header-announcement d-print-none" aria-label="{{ banner_label }}"></aside>`);
fetch("{{ theme_announcement }}")
.then(res => {return res.text();})
.then(data => {
if (data.length === 0) {
console.log("[PST]: Empty announcement at: {{ theme_announcement }}");
return;
}
Comment on lines +31 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm reading this correctly, the aside tag will get written first, even if data.length === 0. Seems like we wouldn't want that to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good point, let me fix it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, just FYI, I rewrote all of this in #1808. Matthias reviewed it. He found one thing that needed to be fixed. I just fixed it. So I'm wondering if it would be better to merge in #1808 and see if it fixes the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks to me that the reason the banner disappeared in #1817 is because the "init" class was getting applied to the <aside> tag when the announcement was local and then never removed.

If that's the case then #1808 should fix #1817.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, PR 1808 has a test case that checks that the banner gets rendered when theme announcement is local.

As an aside, one reason I was holding off on 1808 was that I wanted to see if someone had any thoughts about how to write the tests I added in a more efficient way, but that could always be done in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh - sorry about that, I should have checked. I'll pause here and let you finish 1808 first, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries, I think it's awesome you jumped in to help fix the problem :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@melissawm, I edited some updates you made to the docs about the theme announcement, so if you could double check that my rewrite is accurate that would be awesome

div = document.querySelector(".bd-header-announcement");
div.classList.add(...{{ header_classes | tojson }});
div.innerHTML = `<div class="bd-header-announcement__content">${data}</div>`;
setAnnouncementHeight();
})
.catch(error => {
console.log("[PST]: Failed to load announcement at: {{ theme_announcement }}");
});
} else {
document.write(`<aside class="{{ header_classes | join(' ') }} bd-header-announcement" aria-label="{{ banner_label }}">
<div class="bd-header-announcement__content">{{ theme_announcement }}</div>
</aside>`)
console.log("[PST]: Using local announcement.")
setAnnouncementHeight();
}
</script>
{#- if announcement text is not remote, populate announcement w/ local content -#}
{%- else %}
<aside class="{{ header_classes | join(' ') }} bd-header-announcement"
aria-label="{{ banner_label }}">
<div class="bd-header-announcement__content">{{ theme_announcement }}</div>
</aside>
{% endif %}
Loading