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.

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

Add background colour option to tiles operation (#1923) #1924

Merged
merged 4 commits into from
Oct 25, 2019
Merged

Add background colour option to tiles operation (#1923) #1924

merged 4 commits into from
Oct 25, 2019

Conversation

neave
Copy link
Contributor

@neave neave commented Oct 18, 2019

I've done the best I can to add background colour as an option for the tile() method. I may have missed something here as I'm not a C++ programmer. Let me know your thoughts. Thanks.

@neave
Copy link
Contributor Author

neave commented Oct 18, 2019

Hm it seems the tests fail in regards to skipBlanks, but the PR doesn't touch that code. Any idea what's going on here? Thanks!

@lovell
Copy link
Owner

lovell commented Oct 18, 2019

Thanks for the PR, I think the test failures are due to the need to match the number of channels in the "background" with the number of channels in the image. Try conditionally using pop_back to remove the last element when there is no alpha channel.

@coveralls
Copy link

coveralls commented Oct 21, 2019

Coverage Status

Coverage increased (+0.0003%) to 99.757% when pulling 40f6575 on neave:master into e627f6d on lovell:master.

@neave
Copy link
Contributor Author

neave commented Oct 21, 2019

OK that seemed to do the trick. It can now set the tiles background colour and passes the tests. I've committed the changes but it's now reporting coverage failure?

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thanks for updating, I've left a comment inline about a possible way to deal with the test coverage drop.

lib/output.js Outdated Show resolved Hide resolved
@lovell lovell merged commit 08a6597 into lovell:master Oct 25, 2019
@lovell
Copy link
Owner

lovell commented Oct 25, 2019

Brilliant, thank you Paul.

@lovell lovell added this to the v0.23.2 milestone Oct 25, 2019
lovell added a commit that referenced this pull request Oct 26, 2019
Repository owner deleted a comment from caioaugusto1998 Dec 6, 2019
@ferreiradantas7

This comment was marked as off-topic.

Repository owner locked and limited conversation to collaborators Dec 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants