Problem/Motivation
From a CWV point of view, adding the additional JS and CSS when there are no active alerts and auto-refresh is disabled is sub-optimal
Steps to reproduce
Proposed resolution
Don't attach the library when there are no active alerts and auto-refresh is disabled
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork sitewide_alert-3261893
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
larowlanWas a bit gnarly on the caching front, also fixed what is likely a cache bug with turning the 'show on admin' on and off.
Comment #4
danielvezaLooks good to me! Reducing JS loaded & better cachability is a big win.
Comment #7
chrissnyderMerged and marked fixed! Thanks for the work and testing!
Comment #8
chrissnyderComment #9
chrissnyderI am having second thoughts on this. Mainly because this invalidates full-page caching causing the cacheability of pages to be independent of if there are alerts or not. One of the original goals of this module was to not have alerts impact page caching.
An example demonstrating my concern and why this is important:
A university, school, or government agency needs to publish a very important time-sensitive alert sitewide at a time when traffic to the site is abnormally high. This may be an alert regarding public safety. With this change, adding a sitewide alert would cause the page cache of all the pages on the site to be invalidated (due to the cache tag) resulting in excessive stress on the site as it needs to rebuild all of the pages at a time when traffic is high and the information is important.
One of the main goals of loading the alerts as a separate request was to allow pages to be fully cached independently of active alerts.
Comment #11
larowlanOk - we'll just keep applying the patch to improve our core web vital scores - thanks for considering it
Comment #12
chrissnyderLarowlan,
I did incorporate some of the changes from your work in #3264255: Ensure library attachment is dependent on settings. Thank you for those changes!.
Comment #13
chrissnyderComment #14
larowlanNo worries, we've copied the tests from here into our project and are patching for now.
We'll likely use hook_module_implements_alter to stub out the sitewide_alert_page_top and put it in a block instead
Thanks for this module