Create a favicon for Umami

I created a favicon based on the first letter of the logo.

Does this look okay?

I like this generally as a favicon design. Moving the U nearer the centre would improve it a bit.

I have moved it more central. Tried to get it as close as I can :)

+100 to #5 proposal!

Favicon looks good. If possible, please attach a version with ".ico" extension also

If possible, please attach a version with ".ico" extension also

Good catch! Can someone else chime in on what the current cross-browser file format practice is here? Do we need one of those .ico formats with 16px and 32px variants embedded?

If we're going to abstract the processing out (to favicons.php), we should do that in an OOP way. Working on that now.

Ok, I've re-worked the patch from @markconroy to use OOP.

I've also added a test - 2938190-14-add-favicons--test-only.patch should fail, then
2938190-14-add-favicons.patch should pass.

I had to move the favicon.ico to the root of the theme folder, so that it would be automatically picked up & set to the fallback 'shortcut icon'. I tried to set this using an umami.theme.settings.yml file, but the following failed tests due to a . in the config value:

favicon:
  use_default: false
  path: 'core/profiles/demo_umami/themes/umami/images/favicons/favicon.ico'

Hi here's my review.

1. Patch applies cleanly.

2.

+++ b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
@@ -174,4 +174,21 @@ public function testAccessDeniedToSampleImages() {
+    $result = $this->xpath('//link[@rel = "manifest"]');

+++ b/core/profiles/demo_umami/themes/umami/images/favicons/manifest.json
@@ -0,0 +1,41 @@
\ No newline at end of file

Requires a new line at EOF.

3.

+++ b/core/profiles/demo_umami/themes/umami/src/FaviconHelper.php
@@ -0,0 +1,211 @@
\ No newline at end of file

Another new line at EOF required.

4. favicon.ico appears correctly in the browser.
5. On bookmarking an URL, the 144x144 bookmarked icon is shown.
6. Tests passing in local successfully.
7. Tested on chrome on iPhone 6, Android. Also tested on Edge 16 on Windows 10
8. Markup has correct link elements.

With above nitpicks, I'm good to do RTBC!!
great work guys. :) This looks awesome on the devices I tested on. It might require testing on more devices. :)
so far LGTM.
Thanks!!

helping by removing nit picks by @JayKandari :-)

1. +++ b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
   @@ -185,4 +185,21 @@ public function testAccessDeniedToSampleRecipes() {
   +    $result = $this->xpath('//link[@rel = "shortcut icon"]');
   +    $this->assertEquals(1, count($result), 'The shortcut icon is present.');
   +    $result = $this->xpath('//meta[contains(@name, "msapplication-Tile")]');
   +    $this->assertEquals(2, count($result), 'The ms-application settings are present.');
   +    $result = $this->xpath('//link[@rel = "apple-touch-icon"]');
   +    $this->assertEquals(9, count($result), 'The apple touch icons are present.');
   +    $result = $this->xpath('//link[@rel = "icon"]');
   +    $this->assertEquals(4, count($result), 'The favicon icons are present.');
   +    $result = $this->xpath('//link[@rel = "manifest"]');
   

   nice!

2. +++ b/core/profiles/demo_umami/themes/umami/src/FaviconHelper.php
   @@ -0,0 +1,211 @@
   +    $html_head[] = [
   ...
   +    $html_head[] = [
   ...
   +    $html_head[] = [
   ...
   +    $html_head[] = [
   ...
   +    $html_head[] = [
   ...
   +    $html_head[] = [
   ...
   +    $html_head[] = [
   ...
   +    $html_head[] = [
   ...
   +    $html_head[] = [
   ...
   +    $html_head[] = [
   

   Given we control the theme here - any reason why we don't just hard-code this in markup in html.twig.html?

   hook_page_attachments_alter feels like a hack

#1 nice!

Might have borrowed that idea from core - DefaultMetatagsTest... :)

#2, hardcoding - not sure - @markconroy?

My only argument for doing this via preprocessing is that it means we don't have to bring in a copy of html.html.twig & maintain it with any changes made to classy (the base theme we use). I can't imagine that being a problem often, if at all though. We're also not really modifying the HTML structure as such.

Would be nice to use hook_page_attachments instead of hook_page_attachments_alter if we do stick with this way, but seems like we can only use _alter.

I don't mind which way it's done either.

I'm in favour of "hook_page_attachments_alter" rather than hardcode it on new html.html.twig template. Although it is less likely to change, still we'll be maintaining a new template file. I may be missing something, please correct me.

Marking this RTBC as of now, happy to see others views as well !!

Thanks :)

Instead of thinking of what *we* have to maintain, Umami should be about showing evaluators how *they* can maintain a theme. Using the hook (a) shows off a Drupal alter hook with a render array, and (b) shows that you don't have to override every Twig template in your theme. Umami already has custom templates for page, node, and lots of blocks - nice to leave one of the major ones alone.

Can we also get the asset in the issue here, or better yet a screenshot? I'm assuming it is not actually the Montpellier logo? (Which was an awesome camp but let's not over-borrow their brand.) :) Sorry they are totally here; I just missed them because they were not embedded. I'll update the IS.

Also @larowlan's review from #18 hasn't really been addressed.

Well I agree with @larowlan; we should really avoid using that hook and it seems way off for a theme to use it for its own assets.

At a minimum, it should be refactored so that there is a $icon_base or whatever array that contains the defaults that are used for most of the entries, and have the individual changes added in when it is added to the array. (A for loop might also come in handy.)

Tagging for frontend framework manager review hopefully for some wisdom and guidance. :)

Sorry, I think this is actually something that needs a framework manager review.

@xjm; I don't think changing the multiple $html_head[] = ...s to be a for loop would be any help, as we would still need an array to store the icons – we'd just be copying one array to another.

As a visual test, here's the favicon in Chrome on MacOS:
In the tab –
In the bookmark menu –

And IOS with Safari:
In the bookmark screen –
On the 'home' screen –

There's the issue that when it's put on the home screen, the label defaults to 'App'. This can be changed in the manifest.json file.

It would be nice to get this in 8.6 as it adds a little more pollish to the site.

Have discussed at length with @kjay.

It would be an incremental improvement to include the 32x32 icon uploaded in #5. Therefore we should restrict this issue to achieving that and move talk of manifests and responsive favicons to a follow up issue if required.

Note as core does not support this without custom code, it does not seem appropriate to implement this in Umami, so the follow up might not be required. Let's not debate that here though - if you want to advocate for the approaches discussed from #11 onwards, please create a new issue and link here.

Remaining work is to convert the favicon in #5 to .ico and include it in the theme. Tagging Novice and suggest that's achievable at Drupal Europe.

Gvert and I will be working on this at DrupalEurope.

Based on comment #34, i've created a patch, which adds the favicon.

I'm at DrupalEurope reviewing the patch.

Patch looks good.

Tested on simplytest.me and the favicon is automatically picked up and looks good.

Marking RTBC

Patch applies cleanly. Add the file core/profiles/demo_umami/themes/umami/favicon.ico File looks the same as in issue summary.
Favicon shows up in FF (screenshot below), Chrome, Safari on OSX.

File info, to document the image size:

• Color Model: RGB
• Depth: 8
• Has Alpha: 1
• Pixel Height: 32
• Pixel Width: 32

We could use a follow-up for #34. Feel free to move back to RTBC once that has been done.

Related to the follow-up for #34, there is #2698127: Extend site information page to generate a manifest.json file.

That issue doesn't aim to put icons in the manifest, but it does introduce a hook which can do that. I suggest framing our follow-up as a demo of the core manifest feature, and only do it if that issue lands.

Moving back to RTBC. #34 should not have a follow up until Drupal can implement this in core. I don't think we should raise pre-emptive follow ups in Umami for things core may or may not do.

Superb, a simple favicon is still better than no favicon :) I think it would still be nice to add an issue to support multiple favicons and there is likely one somewhere already.