[4.0] Use preload instead of prefetch alternative #34890
Conversation
joomla-cms-bot
added
NPM Resource Changed
|
for what I thought were fairly obvious reasons the fonts can not be loaded directly from google in the default template. |
I kinda remember #30914 😄 |
|
as it is right now it cant be accepted. |
Then all the Google (or any other remote source) based fonts need to be completely removed as the implementation is totally bs. Wrong usage of prefetch, extra imports, etc it's a disaster... |
|
If the site is behind a firewall then loading a font from google or a script from a cdn will not work To the best of my knowledge there are no assets being used by core, by default, that are being loaded from an external source. If a site owner wants to use scripts from a cdn or fonts from google then that is their choice but it can not be the default. This discussion has been held numerous times and I really dont want to repeat it. |
|
As far as I can see, stuff from Google would only be loaded if one has selected a fonts scheme in template settings which uses Google, but not if "None" or "Roboto (local)" are used, as far as I can see, so that should not be different to what we have now. But if you select a local fonts scheme, the change here hardcodes the font family to "Roboto". That means if someone adds another local font scheme in his or her template copy, he or she has to edit code in the 4 PHP files, while now it just needs to add another (s)css at a special place, which does the includes. It might not be nice like it is now, but it was easier to maintain than it will be with the changes here. |
I could restore the values in the scss for local and adjust the if/else, fixable |
|
@richard67 local fonts now declare their css vars using the scss/css file. |
|
It might need some time until I can test that because I'm currently fighting with another issue with the extensions installer for my latest PR. |
|
@wilsonge a quick decision here? |
|
No need to hurry, if it is not in tomorrow it will be in next time for sure. From review it looked ok to me, but I haven't found time yet for testing. |
|
@dgrammatiko I tested the PR but for some reason the web fonts don't work. On the other hand Roboto local is working like before. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34890. |
|
@richard67 can you share the request header for those pages? |
|
@dgrammatiko FYI the font is loaded in the Chrome inspector network tab but not applied. Probably a CSS issue? This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34890. |
@RickR2H it should be fine now |
|
I have tested this item ✅ successfully on 2faaca4 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34890. |
|
Never mind the comment 😉 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34890. |
|
I have tested this item ✅ successfully on 8158b88 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34890. |
|
I have tested this item ✅ successfully on 8158b88 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34890. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34890. |
joomla-cms-bot
added
RTC
|
@bembelimen can I have a decision here? |
|
As it's targeting 4.0 it's still @wilsonge call, but in my opinion the preload approach is correct, so in general: yes we should go for it.
(Last but not least I'm in general not happy about the inflexible way of setting the fonts in the core templates, it's rather hard to implement own fonts, but that's an issue for another PR I guess) |
I'm really not sure what you mean here, Brian also mentioned that before. Let me explain that this PR is not changing anything fundamental here. Before this PR there was an About the I had a draft PR for localizing the G-Fonts #30914 but people were against it (btw WP is doing that)... |
I fully confirm that this PR changes nothing on the local fonts option and in case of web fonts where they come from, it just eliminates the extra CSS. For the issue with loading web fonts from Google and privacy regulations we have that yellow hint below the "Fonts Scheme" field in the template style's advanced options. |
My bad, didn't check the deleted files. Here are some PHP suggestions: dgrammatiko#3 otherwise it's good to go 👍 |
Minor code cleanup
|
Thx |
|
@richard67 shall I leave the db changes for the update to you or do you want me to try a fix? |
@dgrammatiko Let me do it. I know you can do it, too, but there might be other change being merged before the next release which require files and folder to be deleted, too, and I could that into 1 PR. |
|
#35478 for the deleted files. |
Pull Request for Issue # .
Summary of Changes
Prefetch = load an asset for the next navigation (this is very low priority)
Preload = load an asset with higher priority for the current page
In short, proload is what is needed for the css (color, fonts)
This PR is essentially is implementing basically the https://css-tricks.com/the-fastest-google-fonts/ with some extras:
the webfont will also be available even if Javascript is disabled (the same thing I did for font awesome)
the preload is done in the actual headers of the response
Inlined the CSS Properties for all the fonts
The code is written in a way that a custom google font URL can be used (not exposed to UI, but a simple input and a toggle button will do the trick)
There are changes in the SQL that are not automatically applied
There are SCSS files that were deleted
Testing Instructions
Apply the PR, and make sure to save the Cassiopeia default style!!!
Check all the fonts and the loading (preferably by emulating a very slow connection)
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Documentation Changes Required