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

Corrects 'sample' function overriding 'crop' function #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jhannwong
Copy link

When the code finds that user did not call for 'sample' function, the code correctly assumes the task must do a 'resize'.

Unfortunately, the default behavior ('resize') should have been specified higher up in the code, rather than as an if-else in the check for user-call to 'sample' function.

@andismith
Copy link
Owner

Hi,

Thanks for this, sounds like we should look to fix this. However, the built-in tests for this are currently failing which suggests these changes have changed the expected output.

Have you tried running the test locally to check they pass? Just run grunt in this repo.

Thanks,
Andi

@jhannwong
Copy link
Author

jhannwong commented Jan 30, 2017

@andismith All tests pass locally. grunt nodeunit:tests.
The failed tests seem to indicate unexpected output image resolutions.

Is it possible this bug had caused us to expect the wrong output image resolutions?

@andismith Indeed, this bug had caused us to write the tests wrong. See the 1st test where we expect wrong output image dimensions.

Should not force user-specified size; can exceed image size.
Pointless to crop larger than image size.
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.

None yet

2 participants