Skip to content
This repository has been archived by the owner on Jan 25, 2021. It is now read-only.

Use local Google Fonts #18

Merged
merged 27 commits into from
Aug 30, 2020
Merged

Conversation

drmenzelit
Copy link
Collaborator

@drmenzelit drmenzelit commented Aug 12, 2020

Pull Request for Issue #14 #15 .

Summary of Changes

Added parameter(s) for GoogleFonts.
Example fonts installed.
Removed Fira Sans

Testing Instructions

Open template, you will see two new parameters
Google Font yes / no
Google Font Name as select field

Screenshot_2020-08-12 Templates Edit Style - Joomla Cassiopeia Enhancement - Administration

Expected result

If "no" is selected the template will use the font definitions from BS4 (at the moment)
If "yes" you can select a font or font combination. The selected font will be loaded with a specific css file.

Screenshot_2020-08-12 Home
Screenshot_2020-08-12 Home-Josefin
Screenshot_2020-08-12 Home-nofont

Documentation Changes Required

We need to explain how to install and use other fonts

Update 2020-08-21 by @richard67

There are 2 use cases.

Case 1: CMS contibutors add new fonts to the sources so they are shipped with the CMS.

This should not really happen often since we should at the end ship only the fonts which are really used by the CMS, maybe a choice of 2 which look nice, but not a lot.

Upload a scss file for each font or combination of fonts and corresponding woff and woff2 files to folder /build/media_source/fonts and then run npm ci. That's it.

Compiled css and copied font files will be availabe in folder /media/fonts after that.

In the source code only what has changed or been added in folder /build/media_source/fonts has to be checked in.

Case 2: Users add new fonts to an installlled J4.

Upload a css file for each font or combination of fonts and corresponding woff and woff2 files to folder /media/fonts. Optionally you can also upload the minified css file in addition. That's it.

In both cases, after you have done the above steps (either case 1 or case 2), you can select in backend in Cassiopeia's template style settings the new fonts by the base name of the css files in the /media/fonts folder. Minified css files will be ignored.

Added parameter(s) for GoogleFonts. Example fonts installed.
@drmenzelit drmenzelit marked this pull request as draft August 12, 2020 11:52
renamed scss files, changed if to switch
@richard67
Copy link
Member

@Fedik Hello, maybe you are surprised being pinged from this repository. We are preparing some extensions of the Cassiopeia template, and one of them is providing local Google fonts and preload them. Now my question: The web asset manager which you have added to the core, can it also load fonts? Or do we sill have to use $this->getPreloadManager()->preload(...) like it is done here? https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Document/Document.php#L844.

@Fedik
Copy link
Member

Fedik commented Aug 12, 2020

You still have to use $this->getPreloadManager()

WebAsset just provide an URI to the resource, if you example have an asset for fonts you can:

$wa  = $this->getWebAssetManager();
$wa->useStyle('template.cassiopeia.googlefont');

$this->getPreloadManager()
  ->preload($wa->getAsset('style','template.cassiopeia.googlefont')->getUri(), ['as' => 'style']);

Btw, I forgot to make this change in the joomla core 😃
(So people can override a 'template.cassiopeia.googlefont' with plugin, without editing index.php)

And note about the changes, you better make a local font.css with defined local fonts, and preload this css file, instead of preloading the font files. In this way browser will pick up best font file from the font.css, without loading all of them.

@Fedik
Copy link
Member

Fedik commented Aug 12, 2020

This should work:

switch( $this->params->get('googleFontName'))
{
  case "josefin-sans":
    $wa->useStyle('font.josefin');
    $this->getPreloadManager()->preload($wa->getAsset('style','font.josefin')->getUri(), ['as' => 'style']);
    break;

  case "montserrat":
    $wa->useStyle('font.montserrat');
    $this->getPreloadManager()->preload($wa->getAsset('style','font.montserrat')->getUri(), ['as' => 'style']);
    break;
}

@drmenzelit
Copy link
Collaborator Author

@Fedik thank you for your help. I made the changes as you suggested. The css file is being loaded so

Is it correct or should we expect a rel=preload?

@Fedik
Copy link
Member

Fedik commented Aug 13, 2020

Yes, it is correct.
If you inspect the headers in the dev console (network tab), you should see something like this (but with your font file):

Screenshot_2020-08-13_10-51-29

@hans2103
Copy link
Collaborator

@Fedik I am trying to add the woff and woff2 files to cassiopeia/joomla.asset.json
I get a notice in PHPStorm

Value should be one of: "style", "script", "preset"

Schermafbeelding 2020-08-13 om 10 52 11

I would like to be able to add type: "font",
I would like to see:

<link href="/templates/cassiopeia/font/josefin-sans-v16-latin-600.woff" rel="preload" as="font" type="font/woff2" crossorigin="anonymous">
<link href="/templates/cassiopeia/font/josefin-sans-v16-latin-600.woff2" rel="preload" as="font" type="font/woff2" crossorigin="anonymous">

@hans2103
Copy link
Collaborator

@Fedik sorry.. had to read all of your comments first

And note about the changes, you better make a local font.css with defined local fonts, and preload this css file, instead of preloading the font files. In this way browser will pick up best font file from the font.css, without loading all of them.

@richard67
Copy link
Member

@Fedik Thanks a lot, you've helped much.

@richard67
Copy link
Member

Recent changes due to merged PR's may have broken it. At least I don't see it working here. Will continue to investigate.

@richard67
Copy link
Member

Problem is that $wa->getAsset('style', $googleFontName)->getUri() returns empty.

@richard67
Copy link
Member

The problem is that the compiled css is missing. It seems npm doesn't compile the ./templates/cassiopeia/scss/font--josefin-sans.scss and ./templates/cassiopeia/scss/font--montserrat.scss files.

@richard67
Copy link
Member

richard67 commented Aug 15, 2020

@drmenzelit @hans2103 Ha that was it! The fonts scss files have to be added to the build script ./build/build-modules-js/compilecss.es6.js:

...
        files = [
          `${RootPath}/templates/cassiopeia/scss/offline.scss`,
          `${RootPath}/templates/cassiopeia/scss/font--josefin-sans.scss`,
          `${RootPath}/templates/cassiopeia/scss/font--montserrat.scss`,
          `${RootPath}/templates/cassiopeia/scss/template.scss`,
          `${RootPath}/templates/cassiopeia/scss/template-rtl.scss`,
...

The question is if that it really open for adding fonts later in overrides.

Wouldn't it be better we add css files for the fonts to the css folder of the template instead of scss files which need to be compiled?

@richard67
Copy link
Member

If I could chose, I would make css and put them into a folder "templates/cassiopeia/css/fonts" and remove the "font--" prefix from their name.

Only thing what I don't know is if it needs the step to compile from scss to css for some reason. If that is the case, then I would put the scss files into a folder "templates/cassiopeia/scss/fonts" and also remove the "font--" prefix from their name, and let the script use that folder in the "folders" array and not the single files in the "files" array.

… to the css compile script (#23)

* Rename templates/cassiopeia/scss/font--josefin-sans.scss to templates/cassiopeia/scss/fonts/josefin-sans.scss

* Rename templates/cassiopeia/scss/font--montserrat.scss to templates/cassiopeia/scss/fonts/montserrat.scss

* Fix PHP and SCSS code style

* More PHP code style

* Finish the sub folder move

* Add fonts scss to the compile script
@richard67
Copy link
Member

I've solved it so far. The only question which remains is the one in my review comment above:

#18 (comment)

@drmenzelit Could you check and comment?

@N6REJ
Copy link
Contributor

N6REJ commented Aug 15, 2020

@drmenzelit @hans2103 Ha that was it! The fonts scss files have to be added to the build script ./build/build-modules-js/compilecss.es6.js:

This is not extensible. Either use scss or css. The nice thing about scss is it does allow for the base to be a $var which makes it a single point of change for end-users.
IF the .js can be told to look for 'fonts.scss' & use it if it exists then thats fine. ( pretty sure it can )
Then you'd have fonts.scss call the _variables to sort things out.
Ultimately a .css will be used so there doesn't HAVE to be a .scss as only dev's will have the tools to process it into a .css file anyway.
In the end you want
/css
/fonts

in /css you want
template.css
fonts.css
custom.css

in /fonts you want only your .ttf & .woff/woff2 files.

- Move the fonts abd their scss to `build/media_source/fonts`
- Modify the the css build script so it copies the fonts and the compiled css to `media/fonts`.
- Use a `filelist` field for selecting the fonts => No need to edit `templateDetails.xml` anymore for adding new fonts.
- Dynamically register the asset so no need to edit `joomla.asset.json` anymore for adding new fonts.
Comment on lines 21 to 22
// Template path
$templatePath = Uri::root() . 'templates/'.$this->template;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drmenzelit Do you know if we need this variable? It was added with the first version of the Google Fonts here on this branch. I see it nowhere used now in template.php.

Removed Montserrat / Work Sans combination and added Fira Sans as is in Cassiopeia v1.
@richard67
Copy link
Member

When I open the template style options after a new installation, I see the Google Fonts option is already switched on, and fira-sans is selected. But the frontpage doesn't use that unless I save the template style options, without having changed anything else.

After that first saving, everything works well.

I think it is because the default value of the switcher as defined in the form XML is not equal to what you get in the PHP code when that configuration value is not set.

I'll see if I can fix it, I have an idea.

@brianteeman
Copy link
Contributor

@richard67 that means its not being defined in the installation sql

@richard67
Copy link
Member

richard67 commented Aug 21, 2020

@brianteeman Should it be, or could we use if ($this->params->get('googleFont', true)) instead of if ($this->params->get('googleFont')) here? https://github.com/joomla/cassiopeia/pull/18/files#diff-040c75e93d662244ae546847f23bd32cR41 .. that would work, too, but maybe it's not a good idea.

@brianteeman
Copy link
Contributor

It should be in the sql

@richard67
Copy link
Member

Hmm installation SQL is easy, but making an update SQL to update older 4.0 Betas would maybe be too risky, with search and replace in the options. I have to think about that.

@N6REJ
Copy link
Contributor

N6REJ commented Aug 22, 2020

on a fresh install the default used in the xml is what is set in the db as a param. After the 1st save then it changes to whatever the values in the template are. There's no need to have a value preset in a sql file.
in order to prove this remove all params from the db and reload the template. It will populate based off the defaults. Ran into this many many times building my template a couple of years ago.

@richard67
Copy link
Member

richard67 commented Aug 22, 2020

@N6REJ Sorry Troy, but it seems you did not get the problem. When you make a new installation and you do not do anything in backend and you go to the site, the template checks the result of $this->params->get('googleFont'). And because there is nothing in db and no default given, it returns false (or zero). The frontent template doesn't know anything about the default values of the templates styles form's fields in the backend.

If you now go to backend and check the settings without saving, you will see that the google fonts are swiched on and a font is selected, which does not fit to what you get in the frontent,

There are 2 ways out: 1. Init in db, and 2. using $this->params->get('googleFont', true). But either of these ways means we define the default at 2 places, 1. in the form xml for the field, and 2. in db or the function call.

We had this problem at other places, too, and one of the 2 ways or both have been used to solve it, and we still have it at other places where it was not really solved. I think that's a weak point in design that we define the default values in a way which is not available if not using the form.

@brianteeman
Copy link
Contributor

If as a matter of best practice we get in the habit of always writing the defaults in the xml it will resolve a lot of problems as this "issue" keeps coming up.

Personally I would not care about beta to beta upgrades for this

@richard67
Copy link
Member

richard67 commented Aug 22, 2020

@brianteeman

If as a matter of best practice we get in the habit of always writing the defaults in the xml it will resolve a lot of problems as this "issue" keeps coming up.

Really "xml"? Or should it have been "sql" and the spell checker "corrected" it?

Personally I would not care about beta to beta upgrades for this

Me neither. We have broken it anyway with the child templates , so it's just one more sentence to add to the release notes of the next beta about what to do when updating a previous beta .

@brianteeman
Copy link
Contributor

not enough coffee yet. it was of course sql

@richard67
Copy link
Member

richard67 commented Aug 22, 2020

Regarding the update sql I was silly: No need for a dangerous replace in the update statement. We can simply set the complete params value for the Cassiopeia template style in the same way as we do it for new installations. So at the end when this and other changes are ready, I can make an update SQL to fix params of those template styles.

Update: Forget it, we can't do that. One may alrerady have changed some of the params and will complain if that is reset with an update.

@richard67 richard67 changed the base branch from 4.0-dev to development August 30, 2020 10:51
@richard67 richard67 marked this pull request as ready for review August 30, 2020 10:53
@richard67 richard67 merged commit 1cc000b into development Aug 30, 2020
@richard67
Copy link
Member

Am merging into the development branch for now. For further improvements please make pull requests against the development branch.

@richard67 richard67 deleted the 4.0-dev-google-fonts-enhancements branch August 30, 2020 11:05
@richard67 richard67 mentioned this pull request Sep 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants