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

fix: clamp image dimensions to min and max for snapshot width and minHeight options #460

Merged
merged 1 commit into from
Jan 31, 2020

Conversation

wwilsman
Copy link
Contributor

@wwilsman wwilsman commented Jan 31, 2020

Purpose

#459 brought to light that sometimes image dimensions might be outside of the bounds of our min and max for snapshots.

Approach

Clamp both width and height to between 10px - 2000px, but for snapshot options widths and minHeight only as the image in the DOM should contain the correct width and height.

When we render on our side, the full image should still be captured.

TODO

  • Should I add a matrix of images to test this?
    • Image width < 10px
    • Image width > 2000px
    • Image height < 10px
    • Image height > 2000px
    • Image width < 10px, height < 10px
    • Image width < 10px, height > 2000px
    • Image width > 2000px, height < 10px
    • Image width > 2000px, height > 2000px

@wwilsman wwilsman requested a review from Robdel12 January 31, 2020 18:09
@wwilsman
Copy link
Contributor Author

Hmmm... why are there new snapshots and a missing one? They should all be there 🤔

@Robdel12
Copy link
Contributor

Not sure why the logo is missing, but the new snapshots are because the width is now 812px

Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

Looks like the widths are off 🤔

When we render on our side, the full image should still be captured.
@wwilsman wwilsman force-pushed the ww/clamp-image-upload-dimensions branch from 24b1e4f to d16183f Compare January 31, 2020 19:36
@wwilsman wwilsman requested a review from Robdel12 January 31, 2020 19:37
@wwilsman wwilsman changed the title fix: clamp image dimensions to min and max fix: clamp image dimensions to min and max for snapshot width and minHeight options Jan 31, 2020
Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

🏁

@wwilsman wwilsman merged commit ca68b0f into master Jan 31, 2020
@wwilsman wwilsman deleted the ww/clamp-image-upload-dimensions branch January 31, 2020 20:17
djones pushed a commit that referenced this pull request Jan 31, 2020
## [0.20.13](v0.20.12...v0.20.13) (2020-01-31)

### Bug Fixes

* clamp image dimensions to min and max ([#460](#460)) ([ca68b0f](ca68b0f))
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.

2 participants