fix(gatsby-plugin-manifest): Fix incorrect favicons size bug #12081

keidrun · 2019-02-25T18:20:58Z

Fix #12051 issue

Description

Changed the shape of favicons exported to a square when even providing rectangular images

Related Issues

Fixes #12051

moonmeister

Looks good. According to docs this Shouldn't be needed but might be a bug in sharp related to SVGs. Either way this should fix it.

DSchau

What about a warning?

This seems like this could produce a favicon that a user wouldn't want, e.g. a stretched image.

Maybe better to warn rather than produce something that may not be wanted or is unknown?

moonmeister · 2019-02-28T23:32:53Z

@DSchau According to the docs if you leave out one dimension (as it is without this change) the second one is assumed to be the same. https://sharp.dimens.io/en/stable/api-resize/#parameters My guess is there is a bug with SVGs that makes this untrue.

DSchau · 2019-02-28T23:46:18Z

@moonmeister I don't think that's really how I am understanding that document, and just validated with an example.

Use null or undefined to auto-scale the height to match the width

That means it'll scale both dimensions equally (cover) (e.g. if you had a 50x100 image, if you resized it to 50 height, it'd be 25x50). I think we could tweak this behavior by tweaking the fit option.

Here's an example

Output with width and height
Output with just width

You can see when specifying both width/height it seems to trim the edges.

moonmeister · 2019-02-28T23:54:52Z

@DSchau Ah, okay, I'm glad you actually tested that. Do we need to change the fit option to fix that? Given the default is cover your results make sense. Maybe it should be contain or inside or outside, all seem like they may be what we want.

DSchau · 2019-02-28T23:55:47Z

I don't think we want those! I think we should just warn here, rather than making an opinionated choice for the user.

I suppose we could warn and resize (with one of those options) but either way, I think it should intentionally be more "noisy" here so the user is aware!

moonmeister · 2019-03-01T00:01:23Z

@DSchau I think I'm not understanding why you're concerned about this. I don't mind warning the user if we do something. If a non-square logo is provided it needs to be made square to work...I've never seen a manifest, ios icon, or favicon that wan't square...so I assume this is "expected". Should we warn that they need to provide a square icon or just make it fit into a square with a warning?

DSchau · 2019-03-01T00:04:14Z

@moonmeister sure!

If you accidentally provide a non-square input file (e.g. you take your company's logo.svg, which has dimensions of 100x200), I don't think you'd want behavior of either cutting off or "stretching" your logo to fill the container. If we did default to doing either of those, we should warn when the image is non-square and that it'll be resized.

For the why--branding, brand guidelines, etc. etc.

moonmeister · 2019-03-01T00:09:25Z

@DSchau Okay, Yeah, I think we can scale it (not stretch or cut off) the non-square logo into a square logo (with a warning)...maybe I'm misunderstanding their descriptions of the fit options but that seems like what they do. the Default (cover) cut's off (we don't want this), fill appears to stretch the logo to make it fit, and inside, outside, contain all create an image that's square (based on provided dimensions) with the logo not cut or stretched inside.

DSchau · 2019-03-01T00:38:35Z

contain seems promising! Want to validate with that repo I posted above?

moonmeister · 2019-03-01T00:46:43Z

@DSchau I'm working on some other things atm but if I get time I will

DSchau · 2019-03-11T22:01:57Z

@keidrun any interest in addressing some of the comments above? Specifically:

I'm not sure users will want this behavior automatically and a warning may be better suited
If there is a way to perform this work programmatically (with a nice appearance, e.g. not stretching), then we can do that--just not sure what's possible here!

Thanks for your patience!

keidrun · 2019-03-16T04:25:12Z

@DSchau No such a good idea. I just think the behavior automatically with stretching like http://tools.dynamicdrive.com/favicon/ is better than only warning for me but if most people think a way of a warning is better, we should choose the latter

moonmeister · 2019-03-18T10:28:38Z

@DSchau Okay, I took a couple minutes to do some experimenting with your test!

my recommendation would be we do:

.resize({
      width: 128,
      height: 128,
      fit: "contain",
      background: { r: 255, g: 255, b: 255, alpha: 0 }
    })

We would also throw a warning in the console letting the dev know this expects a square input but we've done our best with what was provided.

This results in containing the provided image within the set dimensions and instead of having black bars it adds transparent bars. This makes the image "look" unchanged while also forcing it to comply.

I was wrong

…s#12051) Fix gatsbyjs#12051 issue

add height and width paramaters to sharp change fit to 'cover' to not malform image set background to be transparent to eliminate black bars add better logging to warn user when src image isn't square.

moonmeister · 2019-03-22T07:56:39Z

@DSchau Rebased, linted, etc. Let me know what you think.

wardpeet

LGTM, just a few nitpicks

packages/gatsby-plugin-manifest/src/gatsby-node.js

moonmeister · 2019-03-22T12:50:03Z

I'll get the tests fixed tomorrow. It's late in Perth.

moonmeister · 2019-03-24T04:36:40Z

@wardpeet FYI, should be good to go.

wardpeet · 2019-03-27T11:31:38Z

thanks a bunch! merging! Keep up the good work! 💪

This is the regular batch update for outdated production and development dependencies. The largest change is the migration to MDX 1.0.0 (1) using the official migration guide for v0 to v1 (2). React has been been updated to the latest patch version 16.8.6 (3) and the `prop-types` package now comes with a handy new `elementType` prop type (4) that can be used for React components. `polished` has been updated to the large 3.0.0 version milestone (5) that comes with many features in form of new modules, improvements like a new error system as well as a a roadmap for v4. The `readableColor` helper now offers the option to set the color(s) it returns for light or dark colors instead of only returning `white` or `black` based on the passed colors luminosity. `stripUnit` now offers the option to return the value and unit as an array, replacing the functionality of `getValueAndUnit` that'll is now deprecated and will be removed in v4. All color modules will now also safely handle the `transparent` keyword instead of erroring out. See the release notes for all details and changes. React Waypoint has been updated to major version 9 (6) that comes with improvements in library size and minifications in form of named exports for the `Waypoint` module as well as for all defined constants. Prettier 1.17.0 (7) now allows to use shared configurations making it much easier to setup new projects and keep the config code base at one single-point-of-truth. Also Markdown tables are now kept compact when reformatting would exceed the print width (8) making largely improving the readability. Gatsby and all official plugins have been updated to the latest versions. This comes with new features that allow environment variables to be replaced per environment (9). `gatsby-plugin-manifest` fixes the incorrect favicons size bug (10) that often appeared as warning in the console. `gatsby-plugin-sharp` now comes with a `defaultQuality` option (11) to define the default quality for processed images instead of only allowing to set the quality through the GraphQL query. `gatsby-image` now comes with a `durationFadeIn` option (12) that accepts a number instead of boolean to customize animation duration. >>>>>> Production Dependencies - @babel/polyfill `7.2.5` -> `7.4.3` - @mdx-js/tag `0.18.0` -> `0.20.3` - gatsby `2.0.75` -> `2.0.117` - gatsby `2.1.4` -> `2.3.29` - gatsby-image `2.0.30` -> `2.0.40` - gatsby-mdx `0.4.0` -> `0.6.2` - gatsby-plugin-canonical-urls `2.0.10` -> `2.0.12` - gatsby-plugin-catch-links `2.0.10` -> `2.0.13` - gatsby-plugin-google-gtag `1.0.13` -> `1.0.16` - gatsby-plugin-lodash `3.0.4` -> `3.0.5` - gatsby-plugin-manifest `2.0.17` -> `2.0.29` - gatsby-plugin-netlify `2.0.9` -> `2.0.15` - gatsby-plugin-offline `2.0.23` -> `2.0.25` - gatsby-plugin-react-helmet `3.0.6` -> `3.0.12` - gatsby-plugin-remove-trailing-slashes `2.0.7` -> `2.0.11` - gatsby-plugin-sharp `2.0.23` -> `2.0.35` - gatsby-plugin-sitemap `2.0.5` -> `2.0.12` - gatsby-plugin-styled-components `3.0.5` -> `3.0.7` - gatsby-plugin-svgr `2.0.1` -> `2.0.2` - gatsby-source-filesystem `2.0.20` -> `2.0.32` - gatsby-source-graphql `2.0.10` -> `2.0.18` - gatsby-transformer-sharp `2.1.15` -> `2.1.18` - gatsby-transformer-yaml `2.1.8` -> `2.1.12` - inter-ui `3.3.2` -> `3.5.0` - polished `2.3.3` -> `3.2.0` - prop-types `15.6.2` -> `15.7.2` - react `16.8.3` -> `16.8.6` - react-dom `16.8.3` -> `16.8.6` - react-pose `4.0.6` -> `4.0.8` - react-spring `8.0.7` -> `8.0.19` - react-waypoint `8.1.0` -> `9.0.2` - semver `5.6.0` -> `6.0.0` - styled-components `4.1.3` -> `4.2.0` - typeface-rubik `0.0.54` -> `0.0.72` >>>>>> Development Dependencies - @babel/core `7.2.2` -> `7.4.3` - @babel/plugin-proposal-class-properties `7.3.0` -> `7.4.0` - @babel/plugin-proposal-nullish-coalescing-operator `7.2.0` -> `7.4.3` - @mdx-js/mdx `0.20.1` -> `1.0.14` - @mdx-js/tag `0.18.2` -> `0.20.3` - @svgr/webpack `4.1.0` -> `4.2.0` - babel-jest `24.1.0` -> `24.7.1` - babel-plugin-react-remove-properties `0.2.5` -> `0.3.0` - babel-preset-gatsby `0.1.7` -> `0.1.11` - eslint `5.14.0` -> `5.16.0` - eslint-plugin-import `2.16.0` -> `2.17.2` - eslint-plugin-react-hooks `1.0.2` -> `1.6.0` - husky `1.3.1` -> `2.1.0` - jest `24.1.0` -> `24.7.1` - jest-dom `3.0.2` -> `3.1.3` - jest-junit `6.2.1` -> `6.3.0` - lint-staged `8.1.3` -> `8.1.5` - prettier `1.16.4` -> `1.17.0` - react-testing-library `5.5.3` -> `6.1.2` - webpack-bundle-analyzer `3.0.3` -> `3.3.2` References: (1) https://mdxjs.com/blog/v1 (2) https://mdxjs.com/migrating/v1 (3) https://github.com/facebook/react/releases/tag/v16.8.6 (4) facebook/prop-types#211 (5) https://github.com/styled-components/polished/releases/tag/v3.0.0 (6) https://github.com/brigade/react-waypoint/releases/tag/v9.0.0 (7) https://prettier.io/blog/2019/04/12/1.17.0.html (8) https://prettier.io/blog/2019/04/12/1.17.0.html#do-not-align-table-contents-if-it-exceeds-the-print-width-and-prose-wrap-never-is-set-5701-by-chenshuai2144 (9) gatsbyjs/gatsby#10565 (10) gatsbyjs/gatsby#12081 (11) gatsbyjs/gatsby@8af9826 (12) gatsbyjs/gatsby#13566 GH-137

keidrun requested a review from moonmeister February 25, 2019 18:44

moonmeister previously approved these changes Feb 28, 2019

View reviewed changes

DSchau reviewed Feb 28, 2019

View reviewed changes

DSchau added the status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged label Feb 28, 2019

moonmeister self-requested a review March 1, 2019 18:10

moonmeister added type: bug An issue or pull request relating to a bug in Gatsby status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Mar 1, 2019

wardpeet requested a review from DSchau March 11, 2019 21:08

keidrun and others added 2 commits March 22, 2019 07:40

fix(gatsby-plugin-manifest): Fix incorrect favicons size bug (gatsbyj…

1087a92

…s#12051) Fix gatsbyjs#12051 issue

fix: non square image issue

5d01ae6

add height and width paramaters to sharp change fit to 'cover' to not malform image set background to be transparent to eliminate black bars add better logging to warn user when src image isn't square.

moonmeister force-pushed the fix-incorrect-favicons-bug branch from 0b0deb3 to 5d01ae6 Compare March 22, 2019 07:53

fix: lint error

8bcd80c

moonmeister removed the status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged label Mar 22, 2019

wardpeet changed the title ~~fix(gatsby-plugin-manifest): Fix incorrect favicons size bug (#12051)~~ fix(gatsby-plugin-manifest): Fix incorrect favicons size bug Mar 22, 2019

wardpeet reviewed Mar 22, 2019

View reviewed changes

packages/gatsby-plugin-manifest/src/gatsby-node.js Outdated Show resolved Hide resolved

packages/gatsby-plugin-manifest/src/gatsby-node.js Outdated Show resolved Hide resolved

packages/gatsby-plugin-manifest/src/gatsby-node.js Outdated Show resolved Hide resolved

moonmeister added 3 commits March 22, 2019 13:16

fix: cleanup warning

f04ca7e

refactor: move to async await

811ecdc

test: fix tests to work with sharp.metadata()

534a327

moonmeister approved these changes Mar 22, 2019

View reviewed changes

moonmeister requested a review from wardpeet March 23, 2019 04:18

wardpeet merged commit 366980b into gatsbyjs:master Mar 27, 2019

arcticicestudio mentioned this pull request Apr 25, 2019

Dependency Update February-April 2019 nordtheme/web#137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gatsby-plugin-manifest): Fix incorrect favicons size bug #12081

fix(gatsby-plugin-manifest): Fix incorrect favicons size bug #12081

keidrun commented Feb 25, 2019

moonmeister left a comment

DSchau left a comment

moonmeister commented Feb 28, 2019 •

edited

Loading

DSchau commented Feb 28, 2019 •

edited

Loading

moonmeister commented Feb 28, 2019

DSchau commented Feb 28, 2019 •

edited

Loading

moonmeister commented Mar 1, 2019

DSchau commented Mar 1, 2019

moonmeister commented Mar 1, 2019

DSchau commented Mar 1, 2019

moonmeister commented Mar 1, 2019

DSchau commented Mar 11, 2019

keidrun commented Mar 16, 2019

moonmeister commented Mar 18, 2019

moonmeister commented Mar 22, 2019

wardpeet left a comment

moonmeister commented Mar 22, 2019

moonmeister commented Mar 24, 2019

wardpeet commented Mar 27, 2019

fix(gatsby-plugin-manifest): Fix incorrect favicons size bug #12081

fix(gatsby-plugin-manifest): Fix incorrect favicons size bug #12081

Conversation

keidrun commented Feb 25, 2019

Description

Related Issues

moonmeister left a comment

Choose a reason for hiding this comment

DSchau left a comment

Choose a reason for hiding this comment

moonmeister commented Feb 28, 2019 • edited Loading

DSchau commented Feb 28, 2019 • edited Loading

moonmeister commented Feb 28, 2019

DSchau commented Feb 28, 2019 • edited Loading

moonmeister commented Mar 1, 2019

DSchau commented Mar 1, 2019

moonmeister commented Mar 1, 2019

DSchau commented Mar 1, 2019

moonmeister commented Mar 1, 2019

DSchau commented Mar 11, 2019

keidrun commented Mar 16, 2019

moonmeister commented Mar 18, 2019

moonmeister commented Mar 22, 2019

wardpeet left a comment

Choose a reason for hiding this comment

moonmeister commented Mar 22, 2019

moonmeister commented Mar 24, 2019

wardpeet commented Mar 27, 2019

moonmeister commented Feb 28, 2019 •

edited

Loading

DSchau commented Feb 28, 2019 •

edited

Loading

DSchau commented Feb 28, 2019 •

edited

Loading