Skip to content

Conversation

@dmnsgn
Copy link
Contributor

@dmnsgn dmnsgn commented May 25, 2020

  • set default to false
  • add platform to html
  • add config icon
  • add to all platform option
  • include apple platform in tests
  • update readme with explanation from webhint

This PR add a new platform: apple and takes care of generating a single icon image for apple devices (<link rel="apple-touch-icon" href="apple-touch-icon.png">. It follows the recommendation from webhint and lighthouse to have a single image instead of the dozen currently generated by the appleIcon platform:

I went the separate platform way instead of a onlyNewAppleSizes option as suggested in @evilebottnawi comment (#130 (comment)) as it seems to be the new norm, it's quite specific to the apple platform and I see it as exclusive from the current appleIcon support that might disappear. Let me know if that makes sense!

Close #130
Close #198

- set default to false
- add platform to html
- add config icon
- add to all platform option
- include apple platform in tests
- update readme with explanation from webhint
@alexander-akait
Copy link
Member

/cc @dmnsgn sorry for delay, friendly ping

({ theme_color, background }) => `<meta name="theme-color" content="${theme_color || background}">`,
({ appName }) => appName ? `<meta name="application-name" content="${appName}">` : `<meta name="application-name">`
],
apple: [
Copy link
Member

Choose a reason for hiding this comment

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

What about do not add new options, and use existing appleIcon? For example:

  • appleIcon: true- generate all icons
  • appleIcon: [] - allow to specify sizes
  • appleIcon: 'minimum' generate only minimum required icons and tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like moving forward, what you have named minimum is actually be the norm and recommended on Apple platforms. So I'd actually go the other way around:

  • appleIcon: 'all' - generate all icons
  • appleIcon: [] - allow to specify sizes
  • appleIcon: true - generate only minimum required icons and tags

Copy link
Member

Choose a reason for hiding this comment

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

I am afraid changing logic for appleIcon: true is will be breaking change, we can do it in the next major release, not right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, isn't this PR #312 solving the problem by allowing to pass appleIcon: ["apple-touch-icon.png"]? I don't think the generated html is updated though (eg. all sizes are still in html, just images are generated) as I don't see any filter here: https://github.com/itgalaxy/favicons/blob/master/src/config/html.js#L12

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to send a fix for #312, appleIcon: true- generate all icons is not generate all icons, we can't change it by default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Require an apple touch icon Apple icons sizes

2 participants