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

Animate remotely loaded banners together #1808

Merged
merged 8 commits into from
May 23, 2024

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented May 10, 2024

The banner animation felt a little funky to me, even after the improvements made in #1693.

My hunch is that because the two banners are stacked on top of each other and the height of one affects the layout/position of the other, trying to animate the height of both of them at the same time causes the browser to stutter. Or maybe it was just because they could each load at different but often only slightly offset times. Whatever the case, I decided to do a little code clean up and change it so that they both come in together.

In the process of working on this PR it also made sense to address a TODO, and add "Version warning" to the list of translatable strings.

Copy link
Collaborator Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

Done self-reviewing

docs/user_guide/announcements.rst Show resolved Hide resolved
</aside>
{% endif %}
{#- The "revealer" allows async banners to be loaded, revealed, and animated together in a controlled way -#}
<div class="pst-async-banner-revealer d-none">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the crux of the change. Instead of rendering the banners in separate places and animating them each separately, I colocate them within the same container, and animate the container instead.

Something to note: the version warning banner is always loaded at run time, whereas the announcement banner might be rendered at build time or run time (if the configuration variable starts with "http")

{#- The "revealer" allows async banners to be loaded, revealed, and animated together in a controlled way -#}
<div class="pst-async-banner-revealer d-none">
{#- Version warning banner is always loaded remotely/asynchronously #}
<aside class="bd-header-version-warning d-none d-print-none" aria-label="{{ _('Version warning') }}"></aside>
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 added d-print-none to the version warning banner because that seems consistent with #1770

</aside>
{% endif %}
{#- The "revealer" allows async banners to be loaded, revealed, and animated together in a controlled way -#}
<div class="pst-async-banner-revealer d-none">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of the asynchronously loaded elements are contained within elements that start with the Bootstrap d-none utility class, which applies the display: none CSS rule.

The last step in loading this remote content is to remove the d-none class; this ensures that they will only appear if everything goes well.

const wantsWarningBanner = DOCUMENTATION_OPTIONS.show_version_warning_banner;

if (hasVersionsJSON && (hasSwitcherMenu || wantsWarningBanner)) {
const data = await fetchVersionSwitcherJSON(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I discovered this line I was a bit surprised. An await in the middle of a module file causes all the rest of the code below it to be deferred until the promise is settled. So this represents a pretty serious slowdown in executing this file, since pretty much all of the code in this file is actually run at the end of the file.

This was introduced in #1344. Previously, the fetch was also executed at the module level, but without the await syntax, so it didn't hold back the rest of the file from executing.

So I decided to put all of this code into a new asynchronous function, fetchAndUseVersions which is called when the document is ready.

if (hasVersionsJSON && (hasSwitcherMenu || wantsWarningBanner)) {
const data = await fetchVersionSwitcherJSON(
DOCUMENTATION_OPTIONS.theme_switcher_json_url,
async function fetchAndUseVersions() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function definition should arguably moved to a different part of the file, next to other code that has to do with the version switcher, but I didn't want to make the diff harder to compare so I left this code at the same place in the file.

width: 100%;
display: flex;
position: relative;
align-items: center;
justify-content: center;
text-align: center;
transition: height 300ms ease-in-out;
overflow-y: hidden;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the transition and overflow rules to the new container element.

Comment on lines -18 to -22
&.init {
position: fixed;
visibility: hidden;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This class was added by #1693. It's not needed now.

@@ -1,50 +1,18 @@
{% set banner_label = _("Announcement") %}
{% set header_classes = ["bd-header-announcement", "container-fluid", "init"] %}
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 removed the Bootstrap container-fluid class because the styles it provides are all now provided or overridden by the styles set directly on the banners in _announcement.scss.

Copy link
Collaborator Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

Done self-reviewing

Comment on lines 8 to 19
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");
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 moved this JavaScript to the js file, which is a better place for it to live.

assert banner.text.strip() == "Hello, world!"


def test_remote_announcement_banner(sphinx_build_factory) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two tests are actually just unit tests for the announcement.html template. We don't really need to go through the sphinx_build_factory; I just don't know how to write this test in another way.

@gabalafou
Copy link
Collaborator Author

While working on this, I couldn't help but wonder if we should give some of this version loading machinery rethink. Could we get the versions at build time?

Copy link
Collaborator

@Carreau Carreau left a comment

Choose a reason for hiding this comment

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

In one place I think you've hardcoded hello world, where it needs to be a variable.

otherwise looks good.

…ouncement.html

Co-authored-by: M Bussonnier <bussonniermatthias@gmail.com>
@drammock
Copy link
Collaborator

xref to #1822 which is touching the same code right now, FYI

@trallard trallard added the kind: enhancement New feature or request label May 19, 2024
@gabalafou gabalafou added the kind: bug Something isn't working label May 20, 2024
@gabalafou gabalafou linked an issue May 20, 2024 that may be closed by this pull request
Copy link

github-actions bot commented May 21, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/pydata_sphinx_theme
  translator.py
Project Total  

This report was generated by python-coverage-comment-action

try {
const response = await fetch(pstAnnouncementUrl);
if (!response.ok) {
throw new Error(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Incorporates #1755

@Carreau Carreau self-requested a review May 22, 2024 13:53
@Carreau
Copy link
Collaborator

Carreau commented May 23, 2024

Ok, let's try this.

@Carreau Carreau merged commit bbe3e57 into pydata:main May 23, 2024
18 checks passed
@gabalafou gabalafou deleted the animate-banners-together branch May 23, 2024 08:27
ivanov pushed a commit to ivanov/pydata-sphinx-theme that referenced this pull request Jun 5, 2024
The banner animation felt a little funky to me, even after the improvements made in pydata#1693.

My hunch is that because the two banners are stacked on top of each other and the height of one affects the layout/position of the other, trying to animate the height of both of them at the same time causes the browser to stutter. Or maybe it was just because they could each load at different but often only slightly offset times. Whatever the case, I decided to do a little code clean up and change it so that they both come in together.

In the process of working on this PR it also made sense to address a TODO, and add "Version warning" to the list of translatable strings.

* pybabel extract . -F babel.cfg -o src/pydata_sphinx_theme/locale/sphinx.pot -k '_ __ l_ lazy_gettext'

* pybabel update -i src/pydata_sphinx_theme/locale/sphinx.pot -d src/pydata_sphinx_theme/locale -D sphinx

* Update src/pydata_sphinx_theme/theme/pydata_sphinx_theme/sections/announcement.html


* incorporate pydata#1755

---------

Co-authored-by: M Bussonnier <bussonniermatthias@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working kind: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No announcement banner anymore
5 participants