Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(gatsby-plugin-manifest): allow multiple icon paths #13059

Merged
merged 4 commits into from
Apr 9, 2019
Merged

fix(gatsby-plugin-manifest): allow multiple icon paths #13059

merged 4 commits into from
Apr 9, 2019

Conversation

me4502
Copy link
Contributor

@me4502 me4502 commented Apr 3, 2019 •
edited by moonmeister
Loading

Description

gatsby-plugin-manifest fails to create images when paths are different.
gatsby-plugin-manifest does not delete the include_favicon option when creating the manifest file.

Related Issues

Fixes #13055

@me4502 me4502 mentioned this pull request Apr 3, 2019
Closed
@me4502 me4502 force-pushed the fix/manifest-icons branch from cbe3fef to c7ea1d4 Compare April 3, 2019 00:34
@me4502 me4502 requested a review from a team April 9, 2019 00:17
@DSchau DSchau changed the title fix(manifest): fixed the manifest containing include_favicon and breaking on multiple icon paths fix(gatsby-plugin-manifest): allow multiple icon paths with include_favicon option Apr 9, 2019
@DSchau DSchau added status: awaiting author response Additional information has been requested from the author status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response and removed status: awaiting author response Additional information has been requested from the author labels Apr 9, 2019
DSchau
DSchau reviewed Apr 9, 2019
View reviewed changes
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

How about a few tests validating this fix?

Also - I'm not sure exactly how this fixes include_favicon?

Thanks!

@DSchau DSchau added status: awaiting author response Additional information has been requested from the author and removed status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Apr 9, 2019
@me4502
Copy link
Contributor Author

me4502 commented Apr 9, 2019

This used to fix include_favicon, however it was fixed in master so my fix got removed when I rebased.

@DSchau DSchau changed the title fix(gatsby-plugin-manifest): allow multiple icon paths with include_favicon option fix(gatsby-plugin-manifest): allow multiple icon paths Apr 9, 2019
@me4502 me4502 added status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response and removed status: awaiting author response Additional information has been requested from the author labels Apr 9, 2019
DSchau
DSchau reviewed Apr 9, 2019
View reviewed changes
packages/gatsby-plugin-manifest/src/__tests__/gatsby-node.js
path.dirname(`other-icons/icon-48x48.png`)
)

const calls = fs.mkdirSync.mock.calls
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const calls = fs.mkdirSync.mock.calls
expect(fs.mkdirSync).toHaveBeenNthCalledWith(1, firstIconPath)
expect(fs.mkdirSync).toHaveBeenNthCalledWith(2, secondIconPath)

I haven't tested this--but just a suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it like that originally, however I modified it to use calls to be inline with the other tests in the file. I can change it back if you'd like

Copy link
Contributor

Choose a reason for hiding this comment

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

@me4502 My 2 cents: calls are probably the less clean way to do this (I wrote some of these tests...I just didn't no this other option existed). This seems much cleaner and robust. Do as you wish imo. Another PR to clean up all the tests is welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it'd make more sense to clean up all tests in a single PR, I can do that instead.

Copy link
Contributor

@DSchau DSchau Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's a fair response to continue with what was in the test/file to begin with--so don't want to dissuade from that behavior.

Let's revisit this in a future PR! For now: 🚀

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷‍♂️ I have no preference. See if @DSchau does. My guess is he won't.

moonmeister
moonmeister approved these changes Apr 9, 2019
View reviewed changes
Copy link
Contributor

@moonmeister moonmeister left a comment

Choose a reason for hiding this comment

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

Dustin's suggestion on cleaning up expect is good. otherwise this looks great. Thanks for the PR.

@DSchau DSchau removed the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Apr 9, 2019
@DSchau DSchau merged commit 5dcde0d into gatsbyjs:master Apr 9, 2019
@me4502
Copy link
Contributor Author

me4502 commented Apr 9, 2019

Thanks, I'll write another PR to cleanup the tests shortly.

@me4502 me4502 deleted the fix/manifest-icons branch April 9, 2019 06:55
@me4502 me4502 mentioned this pull request Apr 9, 2019
Merged
@DSchau
Copy link
Contributor

DSchau commented Apr 9, 2019

Successfully published:

  • gatsby-plugin-manifest@2.0.28

johno pushed a commit to jlengstorf/gatsby that referenced this pull request Apr 10, 2019
@me4502 @johno
fix(gatsby-plugin-manifest): allow multiple icon paths (gatsbyjs#13059)
4bcd73a 
## Description

gatsby-plugin-manifest fails to create images when paths are different.
~~gatsby-plugin-manifest does not delete the include_favicon option when creating the manifest file.~~

## Related Issues

Fixes gatsbyjs#13055
johno pushed a commit to jlengstorf/gatsby that referenced this pull request Apr 10, 2019
@me4502 @johno
test(gatsby-plugin-manifest): cleanup tests (gatsbyjs#13240)
b2e3480 
## Description

Cleaning up a few of the manifest tests, as mentioned in gatsbyjs#13059

## Related Issues

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

@DSchau DSchau DSchau left review comments

@moonmeister moonmeister moonmeister approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@me4502 @DSchau @moonmeister