feat: expose favicon asset path to deployment config

[jfmcquade]

PR Checklist

• [x] PR title descriptive (can be used in release notes)

Description

Exposes a property for setting the path to a custom favicon icon from a deployment config, config.favicon_asset. The path should be relative to the gdrive assets folder, e.g. "images/icons/favicon.svg". (NB, the property is not named "favicon_path", as the values of properties whose names end with "_path" are converted to absolute paths when the deployment config is compiled, which is not desirable here).

If no custom favicon asset is set, then the default is used.

Testing

With this feature branch checked out, add the line config.favicon_asset = "images/icons/favicon.svg" to .idems_app/deployments/debug/config.ts and run yarn workflow deployment set debug to compile the config. Now run yarn workflow sync assets to ensure the debug favicon asset is downloaded. Upon running yarn start, the browser favicon should be the custom icon added here.

Git Issues

Closes #1773

Screenshots/Videos

Browser tab showing the custom favicon icon:

[jfmcquade]

Thanks @chrismclarke, I think nesting the favicon config property in a web property is sensible.

The other minor issue is that initial favicon is defined in the main src\index.html file, which the seo service replaces. ...

As it's defined in src/index.html, I've removed the logic for setting the default path elsewhere.

You're right that the favicon initially loads as default and then changes on initialisation of the SEO service. I can see the other advantages to using a different method, like replacing the assets on sync, shouldn't be too hard if we want to implement at some point.

[chrismclarke]

The other minor issue is that initial favicon is defined in the main src\index.html file, which the seo service replaces. ...

As it's defined in src/index.html, I've removed the logic for setting the default path elsewhere.

I'm not entirely sure what you mean by this - I can see the seo service no longer includes the ${PUBLIC_URL}/assets/icon/favicon.png favicon, however by removing it means that any deployment that doesn't specify the asset will have a broken favicon which isn't ideal (probably bad for SEO, so a bit counter-service namesake).

I've reverted to include default favicon fallback in 7fe67e6, could you just double check the custom favicon still works?

I still think unless there is a reason to replace the favicon at runtime (which we currently have no exposed methods for) it would still make more sense to make the file replacement as part of build process, so would be good to create a small issue that someone could take on post-merge (would be a good first issue).

At the same time it would be good to provide a better default favicon to replace the ionic logo, perhaps someone can come up with a small logo to represent the platform? (possibly side-issue/discussion)

[jfmcquade]

The other minor issue is that initial favicon is defined in the main src\index.html file, which the seo service replaces. ...

As it's defined in src/index.html, I've removed the logic for setting the default path elsewhere.

I'm not entirely sure what you mean by this - I can see the seo service no longer includes the ${PUBLIC_URL}/assets/icon/favicon.png favicon, however by removing it means that any deployment that doesn't specify the asset will have a broken favicon which isn't ideal (probably bad for SEO, so a bit counter-service namesake).

I removed that line from the SEO service because index.html includes the tag <link rel="icon" id="favicon" type="image/png" href="assets/icon/favicon.png" />. So the default favicon path is already explicitly set to "assets/icon/favicon.png", and in the case that no custom path is provided, the SEO service doesn't override it.

But I guess that was a bit of a half-way house between overriding at runtime and not: I can see how, as long as we are doing the runtime override, your changes make the default more explicit. I agree that it seems sensible to do the file replacement as part of the build process instead, I'll make an issue after this PR is merged.

I can confirm that the custom favicon still works with the latest code changes.

And agreed that we should consider a custom default favicon, I've added it to our meeting agenda.

[chrismclarke]

I removed that line from the SEO service because index.html includes the tag <link rel="icon" id="favicon" type="image/png" href="assets/icon/favicon.png" />. So the default favicon path is already explicitly set to "assets/icon/favicon.png", and in the case that no custom path is provided, the SEO service doesn't override it.

Oh interesting, I didn't realise that the service is smart about not overriding what doesn't exist (which I can see now, and yes realise I'm the one who wrote the code 😅 )

But I guess that was a bit of a half-way house between overriding at runtime and not: I can see how, as long as we are doing the runtime override, your changes make the default more explicit. I agree that it seems sensible to do the file replacement as part of the build process instead, I'll make an issue after this PR is merged.

I can confirm that the custom favicon still works with the latest code changes.

And agreed that we should consider a custom default favicon, I've added it to our meeting agenda.

All sounds good, I'd say this should be good to go then