Bundle web component rather than using a CDN #274
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
Eric-Arellano
force-pushed
the
bundle-web-component
branch
from
31062d3
to
312a88d
Compare
Eric-Arellano
changed the title
Eric-Arellano
commented
Apr 20, 2023
There was a problem hiding this comment.
This is ready to go, although we may to land Qiskit/web-components#227 first.
Comment on lines
-36
to
-41
| <style> | ||
| qiskit-ui-shell:not(:defined) { | ||
| display: block; | ||
| margin-bottom: 3.5rem; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
Not needed anymore! The top nav bar will always be defined when the site renders.
| @@ -42,7 +42,6 @@ contents. */ | |||
| --breakpoint-xl: 1200px; | |||
| --font-family-sans-serif: "IBM Plex Sans", Roboto, "Helvetica Neue", Arial, sans-serif; | |||
| --font-family-monospace: "IBM Plex Mono", Consolas, "Courier New", monospace; | |||
| --header-height: 52px; | |||
There was a problem hiding this comment.
This is defined already by the web component. We should not be overriding it.
Eric-Arellano
requested review from
javabster and
HuangJunye
and removed request for
javabster
javabster
reviewed
Apr 25, 2023
Yep, extensively. It's what led to the whole "How we load the file" section in the PR description. It took a few hours of experimenting to get the semantics we want. |
Eric-Arellano
added a commit
that referenced
this pull request
May 4, 2023
## Problem Closes #293. #274 broke some of our CSS by removing the definition of `--header-height` in `theme.css`. While it is true that `top-nav-bar.js` defines the CSS variable `--header-height`, I did not realize that it does not expose the variable globally. It uses a "shadow DOM" for encapsulation. That is a good thing! But, it means we need to define `--header-height` ourselves to use it in our own styling. ## Solution I considered trying to dynamically compute the `--header-height` based on inspecting the `.offsetHeight` of `<qiskit-ui-shell>`. Unfortunately, we can only do this with JavaScript; the (ChatGPT-suggested) code made the project more complex. Instead, I took the simpler approach of copying the value `3.5rem` from `top-nav-bar.js`. While this duplication is bad (Don't Repeat Yourself), we expect the value `3.5rem` to be stable and the complexity of dynamically computing the value seemed even worse.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
We decided to stop loading web components via the Internet (e.g. a CDN like unpkg.com) because:
Related, we also decided to switch from always using the latest version of web components (by always pointing to the same URL) to instead versioning the component. For example, the Sphinx Theme 1.14 might use a different version of the same component than Theme 1.11. We believe it is not very painful to update web components thanks to now having release branches & automated releases. It is much more stable to pin the version of the web component, whereas before, updating the URL impacted every single Theme release without an opportunity to validate it makes sense for each release.
FYI: How we load the file
We use an "immediately invoked function expression" (IIFE). That makes sure that the top nav bar is rendered at the same time as the general site, unlike before. See Qiskit/web-components#227 for the change to have the web-components repo build the file this way.
Rejected: ES6 module via a local file
We'd still use
type="module"like we were using before, but change thehref/srcto point to a local file path rather than URL.This breaks opening up
index.html. We get a CORS issue that the file cannot be accessed. That happens because<script type="module">has stricter CORS policies. So, you now need to use a server like runningpython -m http.serverinside the_build/htmlfolder for the site to render properly.This also still has a delay in the top nav bar rendering. That is because ES6 modules have deferred execution, meaning that the JavaScript runs after the HTML page is set up.
Rejected: inlining the JavaScript module
We can avoid the CORS issue from "ES6 module via a local file" by inlining the JavaScript with a Jinja
includedirective.But, that increases the file size of each HTML page by 184KB. It means that we keep downloading the same duplicate JS code because the browser cannot cache the dedicated web component file.
This approach also still results in a delay in loading the top nav bar because ES6 modules have deferred execution.