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 a new 'precisePreview' option #300

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lexiv0re
Copy link

An attempt to implement a workaround for fengyuanchen/cropper#969.
A new option is proposed to control the desired behavior. Due to the default value compatible with the existing behavior, it does not introduce any breaking changes.
With this option enabled it applies rounding in the calculation of the preview size and position similar to what being applied during the crop process.
Also, fixes inaccuracy (for images with odd height/width) in source canvas transformations (scale/rotate).

Canvas context is translated by width/2, height/2 before applying transformations and should be translated back by the same values, not the rounded ones.
When true it takes rounding errors into account and makes the preview exactly match the cropped result
@daiyam
Copy link

daiyam commented Dec 2, 2020

@lexiv0re Can you resolve the conflicting files because your PR is fixing the issue #551? Thx

@codecov-io
Copy link

Codecov Report

Merging #300 (d6da068) into master (de57fa9) will decrease coverage by 0.35%.
The diff coverage is 46.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
- Coverage   72.63%   72.27%   -0.36%     
==========================================
  Files           9        9              
  Lines        1535     1544       +9     
==========================================
+ Hits         1115     1116       +1     
- Misses        420      428       +8     
Impacted Files Coverage Δ
src/js/preview.js 80.59% <42.85%> (-10.79%) ⬇️
src/js/utilities.js 73.94% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de57fa9...d6da068. Read the comment docs.

@lexiv0re
Copy link
Author

lexiv0re commented Apr 3, 2021

@daiyam I resolved the conflicts but this PR was there for more than 3 years now w/o any attention. So I'm not sure if it's worth investing more time into adding tests as it looks like it may never be merged.

@daiyam
Copy link

daiyam commented Apr 3, 2021

@lexiv0re Thank you for the PR and the update :)

@Axel-Jacobsen
Copy link

Will some version of a fix like this be merged? It seems like it solves some important bugs (e.g. #551).

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

4 participants