[Feat] Enable external serving of Vizro assets #775
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
antonymilne
changed the title
antonymilne
requested review from
stichbury,
Joseph-Perkins,
huong-li-nguyen,
maxschulz-COL and
lingyielia
as code owners
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
I guess these tickets could be closed after merging this:
https://github.com/McK-Internal/vizro-internal/issues/1050 and
https://github.com/McK-Internal/vizro-internal/issues/1267
I think you meant when |
I've tested all above manually and everything seems to work fine 🥳 Just the order of reading in the vizro-bootstrap.min.css file I would change to how it was before. It's safer so that we can ensure overwrites work as expected.
|
This reverts commit fb4540a.
|
@huong-li-nguyen thank you for all the testing! 🙏
Ah that would be a nice bonus! I don't understand these tickets well enough to check unfortunately. Please could you test when you review to see if they're fixed? |
vizro-core/changelog.d/20241002_113350_antony.milne_allow_servering_external_assets.md
Outdated
Show resolved
Hide resolved
vizro-core/changelog.d/20241002_113350_antony.milne_allow_servering_external_assets.md
Outdated
Show resolved
Hide resolved
vizro-core/changelog.d/20241002_113350_antony.milne_allow_servering_external_assets.md
Show resolved
Hide resolved
So, there are two tickets that I thought are solved with this PR: The problem was that user's I guess that the Here's an example on how it can be tested. Let's say that you want to disable automatically collapsing of the nav_panel when the screen's height/width is less than 576px (the behaviour that's implemented within the From the window.dash_clientside.clientside.collapse_nav_panel = function (n_clicks, is_open) {
return [
true,
{
transform: "rotate(0deg)",
transition: "transform 0.35s ease-in-out",
},
"Hide Menu",
];
}From the So, when this PR is merged, we can close the Issue -> https://github.com/McK-Internal/vizro-internal/issues/1267 FYI @nadijagraca This issue is not introduced with Vizro, and it exists in the dash app as well (for multi page apps). Also, this issue is not solved with this PR (and probably can't be solved because it's not the issue at all 😄). The issue says that if you, for example, add something like So, when this PR is merged, we should not close the Issue -> https://github.com/McK-Internal/vizro-internal/issues/1050 |
|
Thanks very much for the careful reviewing @petar-qb and @huong-li-nguyen, much appreciated! I wrote some tests in 1ac4535 and now it's merging |
|
Phew, I really thought I had got this right before but there was a screenshot test failing and so I had to go back again to change the order slightly 😬 The order Dash inserts stylesheets is always:
The problem was that So now what we do is remove the library stylesheets in Here's the results. Crucially Vizro app with
|
Description
serve_locally=False(previously this was possible only for the library CSS, which was justfigures.css)<link rel="stylesheet">in our HTML sourceTo do:
serve_locally=TrueVizro app (try it disconnected from Internet)serve_locally=FalseVizro appserve_locally=TrueDash app using Vizro libraryserve_locally=FalseDash app using Vizro library (less important to test, because it's an edge case and also if the above work then I'm confident this does too)Notes (will go in updated developer docs)
When
serve_locally=True(the default), Dash serves component library resources (generally CSS and JS) through the Flask server using the_dash-component-suitesroute.Vizro()is instantiated.This makes our footprint as small as possible and ensures there's reduced risk of CSS name clash when someone wants to use Vizro just as a library but doesn't instantiate
Vizro()(not common at all now, but maybe will be in the future).When
serve_locally=False, Dash switches to serving resources throughexternal_url, where it's specified. For Vizro components we use jsDeliver as a CDN for this.A few complications:
<script>or<link>in the HTML source. This is achieved withdynamic=Truevizro-bootstrap.min.css)import/exportin themIn future:
external_stylesheetsthat points to the stylesheet on our CDN or download the file to their ownassetsfolder. We'd have a shortcut for this likevizro.themeor do it through bootswatch if that's possible..min.jsrather than just relying on the CDN to minify it. This would let us write "proper" rather than just in-browser JS and mean we benefit from tree-shaking etc. rather than just minification that the CDN does. In reality the optimisations would make very little difference to performance, but it's kind of the "right" way to do things. It's more effort than it's worth to set up at the moment, but if we end up maintaining a bigger JS codebase we might do itNotice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":