Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Animate remotely loaded banners together #1808
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
Changes from 1 commit
f1344c7
8cb830f
b048897
ca95ef6
d663d11
5fc1df3
ae52d97
613527b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.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.
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.
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.
Previously the error was not being printed to the console. I thought it might be helpful to our users to print the error.
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.
Moved the transition and overflow rules to the new container element.
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.
This class was added by #1693. It's not needed now.
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.
This functionality—not showing the banner if it's empty—is now taken care of with the changes from #1703 and with the
d-none
class used in this PR.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 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
.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.
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")
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.
All of the asynchronously loaded elements are contained within elements that start with the Bootstrap
d-none
utility class, which applies thedisplay: 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.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.
Open to name suggestions, since I'm not sure that
pst-async-banner-revealer
is very clearThere 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 added
d-print-none
to the version warning banner because that seems consistent with #1770There 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.
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.