Animate remotely loaded banners together #1808

gabalafou · 2024-05-10T15:52:53Z

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.

gabalafou

Done self-reviewing

docs/user_guide/announcements.rst

gabalafou · 2024-05-10T15:57:35Z

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

-  </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">


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")

gabalafou · 2024-05-10T15:58:09Z

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

+{#- 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>


I added d-print-none to the version warning banner because that seems consistent with #1770

gabalafou · 2024-05-10T16:01:54Z

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

-  </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">


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.

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

gabalafou · 2024-05-10T16:29:02Z

src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js

-const wantsWarningBanner = DOCUMENTATION_OPTIONS.show_version_warning_banner;
-
-if (hasVersionsJSON && (hasSwitcherMenu || wantsWarningBanner)) {
-  const data = await fetchVersionSwitcherJSON(


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.

gabalafou · 2024-05-10T16:30:29Z

src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js

-if (hasVersionsJSON && (hasSwitcherMenu || wantsWarningBanner)) {
-  const data = await fetchVersionSwitcherJSON(
-    DOCUMENTATION_OPTIONS.theme_switcher_json_url,
+async function fetchAndUseVersions() {


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.

gabalafou · 2024-05-10T16:32:10Z

src/pydata_sphinx_theme/assets/styles/sections/_announcement.scss

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


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

gabalafou · 2024-05-10T16:33:33Z

src/pydata_sphinx_theme/assets/styles/sections/_announcement.scss

-  &.init {
-    position: fixed;
-    visibility: hidden;
-  }
-


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

gabalafou · 2024-05-10T16:35:52Z

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

@@ -1,50 +1,18 @@
-{% set banner_label = _("Announcement") %}
-{% set header_classes = ["bd-header-announcement", "container-fluid", "init"] %}


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.

gabalafou

Done self-reviewing

gabalafou · 2024-05-10T16:40:10Z

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

-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");


I moved this JavaScript to the js file, which is a better place for it to live.

gabalafou · 2024-05-10T16:42:41Z

tests/test_build.py

+    assert banner.text.strip() == "Hello, world!"
+
+
+def test_remote_announcement_banner(sphinx_build_factory) -> None:


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.

…nx.pot -k '_ __ l_ lazy_gettext'

…data_sphinx_theme/locale -D sphinx

gabalafou · 2024-05-10T16:48:13Z

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?

Carreau

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

otherwise looks good.

docs/user_guide/announcements.rst

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

…ouncement.html Co-authored-by: M Bussonnier <bussonniermatthias@gmail.com>

drammock · 2024-05-17T14:25:58Z

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

github-actions · 2024-05-21T08:40:43Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
src/pydata_sphinx_theme
translator.py
Project Total

_{This report was generated by python-coverage-comment-action}

gabalafou · 2024-05-21T11:02:37Z

src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js

+  try {
+    const response = await fetch(pstAnnouncementUrl);
+    if (!response.ok) {
+      throw new Error(


Incorporates #1755

Carreau · 2024-05-23T08:25:29Z

Ok, let's try this.

Animate remotely loaded banners together

f1344c7

gabalafou commented May 10, 2024

View reviewed changes

gabalafou added 2 commits May 10, 2024 18:44

pybabel extract . -F babel.cfg -o src/pydata_sphinx_theme/locale/sphi…

8cb830f

…nx.pot -k '_ __ l_ lazy_gettext'

pybabel update -i src/pydata_sphinx_theme/locale/sphinx.pot -d src/py…

b048897

…data_sphinx_theme/locale -D sphinx

Carreau requested changes May 14, 2024

View reviewed changes

docs/user_guide/announcements.rst Show resolved Hide resolved

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/sections/announcement.html Show resolved Hide resolved

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/sections/announcement.html Outdated Show resolved Hide resolved

Update src/pydata_sphinx_theme/theme/pydata_sphinx_theme/sections/ann…

ca95ef6

…ouncement.html Co-authored-by: M Bussonnier <bussonniermatthias@gmail.com>

gabalafou mentioned this pull request May 17, 2024

Fix display of local announcement banner #1822

Closed

lint and whitespace

d663d11

trallard added the kind: enhancement New feature or request label May 19, 2024

gabalafou added the kind: bug Something isn't working label May 20, 2024

gabalafou linked an issue May 20, 2024 that may be closed by this pull request

No announcement banner anymore #1817

Closed

gabalafou added 2 commits May 21, 2024 13:00

incorporate pydata#1755

5fc1df3

Merge branch 'main' into animate-banners-together

ae52d97

gabalafou commented May 21, 2024

View reviewed changes

Merge branch 'main' into animate-banners-together

613527b

Carreau self-requested a review May 22, 2024 13:53

Carreau approved these changes May 22, 2024

View reviewed changes

Carreau merged commit bbe3e57 into pydata:main May 23, 2024
18 checks passed

gabalafou deleted the animate-banners-together branch May 23, 2024 08:27

gabalafou mentioned this pull request May 24, 2024

Set banner height to auto after animation #1830

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Animate remotely loaded banners together #1808

Animate remotely loaded banners together #1808

gabalafou commented May 10, 2024 •

edited

gabalafou left a comment

gabalafou May 10, 2024

gabalafou May 10, 2024

gabalafou May 10, 2024

gabalafou May 10, 2024

gabalafou May 10, 2024

gabalafou May 10, 2024

gabalafou May 10, 2024

gabalafou May 10, 2024

gabalafou left a comment

gabalafou May 10, 2024

gabalafou May 10, 2024

gabalafou commented May 10, 2024

Carreau left a comment

drammock commented May 17, 2024

github-actions bot commented May 21, 2024 •

edited

gabalafou May 21, 2024

Carreau commented May 23, 2024

		@@ -1,50 +1,18 @@
		{% set banner_label = _("Announcement") %}
		{% set header_classes = ["bd-header-announcement", "container-fluid", "init"] %}

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


		def test_remote_announcement_banner(sphinx_build_factory) -> None:

Animate remotely loaded banners together #1808

Animate remotely loaded banners together #1808

Conversation

gabalafou commented May 10, 2024 • edited

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou commented May 10, 2024

Carreau left a comment

Choose a reason for hiding this comment

drammock commented May 17, 2024

github-actions bot commented May 21, 2024 • edited

Coverage report

Choose a reason for hiding this comment

Carreau commented May 23, 2024

gabalafou commented May 10, 2024 •

edited

github-actions bot commented May 21, 2024 •

edited