-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: improve quality of image before sending to server #3680
Conversation
@@ -32,7 +32,7 @@ export default ModalBase.extend({ | |||
this.onVisible(); | |||
}, | |||
cropImage() { | |||
this.$('img').croppie('result', 'base64', 'original', 'jpeg').then(result => { | |||
this.$('img').croppie('result', 'base64', 'original', 'jpeg', 1).then(result => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this one, any official guide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quality parameter in croppie .
Link to official guide ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quality parameter in croppie . |
@kushthedude i have migrated the cropper-madal code to ES6 , should i open a seperate PR for that ? |
Yes
…On Mon, 2 Dec, 2019, 20:04 Sundaram Dubey, ***@***.***> wrote:
@kushthedude <https://github.com/kushthedude> i have migrated the
cropper-madal code to ES6 , should i open a seperate PR for that ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3680?email_source=notifications&email_token=AKQMTLVYVEUHSAOPSQJKXPTQWUMHXA5CNFSM4JTVDHD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFTVY4A#issuecomment-560422000>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKQMTLQGJWC6JV2UMMIMIFTQWUMHXANCNFSM4JTVDHDQ>
.
|
@@ -94,7 +94,7 @@ | |||
icon='camera' | |||
hint=(t 'Select Event Image') | |||
maxSizeInKb=10000 | |||
helpText=(t 'We recommend using at least a 2160x1080px (2:1 ratio) image') | |||
helpText=(t 'We recommend using at least a 2160x1080px (2:1 ratio) image and never less than 1500x750px image') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have already specified the at least, This change is unneeded.
Default is already 1, Why define explicitly then?
…On Thu, 5 Dec, 2019, 19:26 Sundaram Dubey, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/components/modals/cropper-modal.js
<#3680 (comment)>
:
> @@ -32,7 +32,7 @@ export default ModalBase.extend({
this.onVisible();
},
cropImage() {
- this.$('img').croppie('result', 'base64', 'original', 'jpeg').then(result => {
+ this.$('img').croppie('result', 'base64', 'original', 'jpeg', 1).then(result => {
https://foliotek.github.io/Croppie/
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3680?email_source=notifications&email_token=AKQMTLSDVX2UEDUFUI2YYWTQXECCRA5CNFSM4JTVDHD2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCODEBJY#discussion_r354326063>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKQMTLUVH5SKHXF7U4H2BGDQXECCRANCNFSM4JTVDHDQ>
.
|
i got that it's affecting the quality a bit , if you say i will remove it . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also dont change the factor, Simply change the result method for the images used to this :
'viewport' the size of the resulting image will be the same width and height as the viewport
'original' the size of the resulting image will be at the original scale of the image
Just change the viewport to the original
@@ -32,7 +32,7 @@ export default ModalBase.extend({ | |||
this.onVisible(); | |||
}, | |||
cropImage() { | |||
this.$('img').croppie('result', 'base64', 'original', 'jpeg').then(result => { | |||
this.$('img').croppie('result', 'base64', 'original', 'jpeg', 1).then(result => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this change
. |
it''s not making any diffrence in quality and returning that poor quality again . when i am changing like this - |
@maze-runnar I think you will need to upgrade our croppie version too. Also, The following solution should work. |
@kushthedude should i update cropipie to 2.6.4 https://www.npmjs.com/package/croppie/v/2.6.4 |
Sure just make sure there are not major changes in them, or else it may
require you a greater amount of work and time for.
…On Sat, 14 Dec, 2019, 22:49 Sundaram Dubey, ***@***.***> wrote:
@kushthedude <https://github.com/kushthedude> should i update cropipie to
2.6.4 https://www.npmjs.com/package/croppie/v/2.6.4
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3680?email_source=notifications&email_token=AKQMTLUJIF4EFH7YTCBNM2DQYUISHA5CNFSM4JTVDHD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4HCPA#issuecomment-565735740>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKQMTLTMBSILKAYZOZOJXZDQYUISHANCNFSM4JTVDHDQ>
.
|
|
Did you check if there is any syntax changes in the new version? As in new
we have to define result and follow the method I told you
…On Sat, Dec 14, 2019 at 11:45 PM Sundaram Dubey ***@***.***> wrote:
[image: Screenshot from 2019-12-14 23-42-21]
<https://user-images.githubusercontent.com/56407566/70852794-adb90680-1ecb-11ea-84d6-96025b2fdbe1.png>
[image: Screenshot from 2019-12-14 23-44-03]
<https://user-images.githubusercontent.com/56407566/70852795-adb90680-1ecb-11ea-9cd5-10cdb1eca4fc.png>
@kushthedude <https://github.com/kushthedude> i have upgraded the croppie
but still no difference
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3680?email_source=notifications&email_token=AKQMTLSNO5R7PEID43RJ42LQYUPDPA5CNFSM4JTVDHD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4IB4A#issuecomment-565739760>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKQMTLVSPY5RSYR5VVHC3ULQYUPDPANCNFSM4JTVDHDQ>
.
|
@kushthedude please review |
@@ -32,7 +32,7 @@ export default ModalBase.extend({ | |||
this.onVisible(); | |||
}, | |||
cropImage() { | |||
this.$('img').croppie('result', 'base64', 'original', 'jpeg').then(result => { | |||
this.$('img').croppie('result', { type: 'base64', size: 'original', quality: 1 }).then(result => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maze-runnar Why did you remove the jpeg
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's not needed.and it was working fine so i removed it .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please restore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
package.json
Outdated
@@ -136,7 +136,7 @@ | |||
}, | |||
"private": true, | |||
"dependencies": { | |||
"@bower_components/Croppie": "Foliotek/Croppie#^2.4.1", | |||
"@bower_components/Croppie": "Foliotek/Croppie#^2.6.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maze-runnar You sure there will be no changes to yarn.lock
for this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes , i don't made any changes beside this in my local . So i am sure .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert it, not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maze-runnar One last thing, Just to be on the safe side, Can you please upload a video or GIF showing the images are not scaling down then we can merge it.
@kushthedude please merge. |
Co-Authored-By: Kush Trivedi <[email protected]>
@kushthedude Please prepare a release |
Fixes #3663
it is resizing the image to higher pixel before sending to server
Checklist
development
branch.