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.

Sign up for GitHub

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

Jump to bottom

fix: introducing cropper in session speaker form for speaker #3819

Merged
merged 8 commits into from Jan 24, 2020
Merged

fix: introducing cropper in session speaker form for speaker #3819

merged 8 commits into from Jan 24, 2020

Conversation

maze-runnar
Copy link
Contributor

Fixes #3794
rescaling the images to 300x300 through cropper.
what it fixes : https://youtu.be/dwNEKSJfba8

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@auto-label auto-label bot added the fix label Jan 18, 2020
kushthedude
kushthedude suggested changes Jan 18, 2020
View reviewed changes
Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

Most of the changes are useless and duplicated.
We already have this component in our app, Please relook and use the image cropper !

@maze-runnar
Copy link
Contributor Author

@kushthedude , i think we can't use that cropper model, as that crops image in 2:1 300x150. And we need 300 x 300px. And i don't think it's a good idea to messup that cropper that is used for event images , as that is a core cropper.

@kushthedude
Copy link
Member

@kushthedude , i think we can't use that cropper model, as that crops image in 2:1 300x150. And we need 300 x 300px. And i don't think it's a good idea to messup that cropper that is used for event images , as that is a core cropper.

Duplication is worse

@maze-runnar
Copy link
Contributor Author

@kushthedude , i think we can't use that cropper model, as that crops image in 2:1 300x150. And we need 300 x 300px. And i don't think it's a good idea to messup that cropper that is used for event images , as that is a core cropper.

Duplication is worse

ok, trying to resole it .

@maze-runnar
Copy link
Contributor Author

@kushthedude please review, removed duplication very much as possible.

@kushthedude
Copy link
Member

kushthedude commented Jan 18, 2020 •
edited

@maze-runnar Just confirm if I am correct about the dimensions(300*300) required for proper rendering of Speaker Image !

@kushthedude
Copy link
Member

kushthedude commented Jan 18, 2020 via email

@maze-runnar
Copy link
Contributor Author

@kushthedude , final video after making changes : https://youtu.be/dlgZSIjnGdI

@maze-runnar
Copy link
Contributor Author

Why do you need to check if it is speaker form or not?
…
On Sat, 18 Jan, 2020, 22:17 Sundaram Dubey, @.> wrote: @.* commented on this pull request. ------------------------------ In app/components/widgets/forms/image-upload.js <#3819 (comment)> : > @@ -8,6 +8,7 @@ export default Component.extend({ selectedImage : null, allowDragDrop : true, requiresDivider : false, + speakerimg : false, @kushthedude https://github.com/kushthedude , it is needed , added in imageupload to check if it is in speaker form or not. Other wise have to write a whole computed property. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3819?email_source=notifications&email_token=AKQMTLX5KYL4NZ65UX6JHL3Q6MXAZA5CNFSM4KISNYBKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSHYXFY#discussion_r368236254>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLUVYMTMWVP2LSRGS7LQ6MXAZANCNFSM4KISNYBA .

oh ! got that , i was checking it when i had made two croppers now i have only one cropper-modal so no need to check .

@kushthedude
Copy link
Member

kushthedude commented Jan 19, 2020 via email

@maze-runnar
Copy link
Contributor Author

Ya , its easy to write code but writing easy code is hard. thanks for continues suggestions.

@kushthedude
Copy link
Member

Final video showing the cropper is workin ?

@maze-runnar
Copy link
Contributor Author

Final video showing the cropper is workin ?

https://youtu.be/XQ-ufoAecFw

@kushthedude
Copy link
Member

Squash the commits, if possible.

@kushthedude kushthedude removed the WIP label Jan 19, 2020
kushthedude
kushthedude previously approved these changes Jan 19, 2020
View reviewed changes
@iamareebjamal
Copy link
Member

Implement center cropping for old pics

@maze-runnar
Copy link
Contributor Author

Implement center cropping for old pics

@iamareebjamal ,it should be done in this PR , or another ?
As It would need to revert later or not for future events(After cropper has been implemented)?

@iamareebjamal
Copy link
Member

iamareebjamal commented Jan 19, 2020 •
edited

No it will not be reverted

@maze-runnar
Copy link
Contributor Author

@iamareebjamal it's not looking efficient to use center cropping , it is degrading the image quality very much, and not working properly too .
Screenshot from 2020-01-20 09-23-41

@kushthedude
Copy link
Member

kushthedude commented Jan 23, 2020 via email

@maze-runnar
Copy link
Contributor Author

Definitely 300 is more better, introduce some margin between them.
…
On Thu, 23 Jan, 2020, 21:32 Sundaram Dubey, @.***> wrote: @kushthedude https://github.com/kushthedude please approve and merge . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3819?email_source=notifications&email_token=AKQMTLRIJVUII47RNMQKHHDQ7G5P5A5CNFSM4KISNYBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJX3QEI#issuecomment-577746961>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLWNIAL4T2QT62HUTN3Q7G5P5ANCNFSM4KISNYBA .

margin won't work as the right side space if for external link and other things and defining margin , messed everything up for mobile screen.

@kushthedude
Copy link
Member

kushthedude commented Jan 23, 2020 via email

@maze-runnar
Copy link
Contributor Author

Will test tonight and let you know if 220 is the only option. On Thu, 23 Jan, 2020, 21:54 Sundaram Dubey, notifications@github.com wrote:
…
Definitely 300 is more better, introduce some margin between them. … <#m_-2471622681149532336_> On Thu, 23 Jan, 2020, 21:32 Sundaram Dubey, @.***> wrote: @kushthedude https://github.com/kushthedude https://github.com/kushthedude please approve and merge . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3819 <#3819>?email_source=notifications&email_token=AKQMTLRIJVUII47RNMQKHHDQ7G5P5A5CNFSM4KISNYBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJX3QEI#issuecomment-577746961>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLWNIAL4T2QT62HUTN3Q7G5P5ANCNFSM4KISNYBA . margin won't work as the right side space if for external link and other things and defining margin , messed everything up for mobile screen. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3819?email_source=notifications&email_token=AKQMTLTXZIVRLPPFIAZKD2TQ7HAEVA5CNFSM4KISNYBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJX6A5I#issuecomment-577757301>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLWTEXU3N2Z6HMPB5MTQ7HAEVANCNFSM4KISNYBA .

may be this will make more clear , the width of accordian -
Screenshot from 2020-01-23 21-56-38

@maze-runnar
Copy link
Contributor Author 

@kushthedude , after adding margin right = 10px . 
.thumbnail-square {
  position: relative;
  width: 300px;
  height: 300px;
  overflow: hidden;
  margin-right:10px;
}

Screenshot from 2020-01-23 22-05-15

kushthedude
kushthedude suggested changes Jan 24, 2020
View reviewed changes
Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

  • Mobile view has lot of empty space.
  • Not all old images are getting scaled, there is still an issue of uneven widths. See here:

Screenshot 2020-01-24 at 1 07 05 PM

- There are issues of scss lint warning, Fix them too.

@kushthedude
Copy link
Member

SCSS Warning if you cant replicate are :

styles/components/speaker-list.scss
  19:11  warning  Space expected after `:`             space-after-colon
  25:3   warning  Expected `height`, found `position`  property-sort-order
  27:3   warning  Expected `position`, found `top`     property-sort-order
  28:3   warning  Expected `top`, found `height`       property-sort-order
  29:3   warning  Expected `transform`, found `width`  property-sort-order
  30:3   warning  Expected `width`, found `transform`  property-sort-order

✖ 6 problems (0 errors, 6 warnings)

@maze-runnar
Copy link
Contributor Author

  • Mobile view has lot of empty space.
  • Not all old images are getting scaled, there is still an issue of uneven widths. See here:
Screenshot 2020-01-24 at 1 07 05 PM

I can't see this type of error oob my local computer, every image is fine .also what about the image size , do you find the way to replace 220 px to 300 as suggested yesterday.

@kushthedude
Copy link
Member

kushthedude commented Jan 24, 2020 via email

@maze-runnar
Copy link
Contributor Author

This error should be on every computed see the css properties width is set to auto hence it will automatically adjust it, and images will have different width which abides the complete sense of center cropping. Test your center cropping on old images uploaded not the new one and if you dont have sufficient data go to Summit 2020 speaker page.
…
On Fri, 24 Jan, 2020, 14:00 Sundaram Dubey, @.***> wrote: - Mobile view has lot of empty space. - Not all old images are getting scaled, there is still an issue of uneven widths. See here: [image: Screenshot 2020-01-24 at 1 07 05 PM] https://user-images.githubusercontent.com/44091822/73052156-7403f400-3eaa-11ea-9bb8-a0ee670e2fdd.png I can't see this type of error oob my local computer, every image is fine .also what about the image size , do you find the way to replace 220 px to 300 as suggested yesterday. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3819?email_source=notifications&email_token=AKQMTLVSKI45U2LR3GURRYTQ7KRJLA5CNFSM4KISNYBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ2CQWY#issuecomment-578037851>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLRD2FXJJGUFTMU4QSDQ7KRJLANCNFSM4KISNYBA .

@kushthedude i think i have to use , following for speaker image besides cropping.
speakerImg {
height: 220px;
width: 220px;
}

@kushthedude
Copy link
Member

Leave the image size case, That's very rare just checked on summit 2020 website only 1 out of 10 images was misaligned. IMO just fix the warnings and mobile spacing.
@iamareebjamal Your opinion ?

@maze-runnar
Copy link
Contributor Author

@kushthedude is it ok?

Screenshot from 2020-01-24 15-49-11

@kushthedude
Copy link
Member

kushthedude commented Jan 24, 2020 via email

@maze-runnar
Copy link
Contributor Author

It looks nice.
…
On Fri, 24 Jan, 2020, 15:51 Sundaram Dubey, @.***> wrote: @kushthedude https://github.com/kushthedude is it ok? [image: Screenshot from 2020-01-24 15-49-11] https://user-images.githubusercontent.com/56407566/73061985-5c843580-3ec1-11ea-974e-9cf36129a27a.png — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3819?email_source=notifications&email_token=AKQMTLWS6TLAHEUBDLBNUJ3Q7K6KXA5CNFSM4KISNYBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ2K3EY#issuecomment-578071955>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLS2FZDPGCY6RJ7KW3DQ7K6KXANCNFSM4KISNYBA .

but i have to apply a seperate class for this -
.mobile-center-cropped {
width: 330px;
height: 330px;
background-position: center center;
background-repeat: no-repeat;
overflow: hidden;
}

@maze-runnar
Copy link
Contributor Author

<div class = "{{if device.isMobile 'mobile-center-cropped' 'center-cropped'}}">

@kushthedude
Copy link
Member

kushthedude commented Jan 24, 2020 via email

@maze-runnar
Copy link
Contributor Author

This is not optimal solution.
…
On Fri, 24 Jan, 2020, 15:56 Sundaram Dubey, @.***> wrote:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3819?email_source=notifications&email_token=AKQMTLRGOZHGZXSA4YA5SXTQ7K63TA5CNFSM4KISNYBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ2LHPQ#issuecomment-578073534>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLSSMOSX3QZ6JNFCT5DQ7K63TANCNFSM4KISNYBA .

i know, but if apply center cropping then for mobile width is also restricted to 220 px.

@kushthedude
Copy link
Member

@maze-runnar Open an issue for follow up CSS improvisations

@maze-runnar
Copy link
Contributor Author

@maze-runnar Open an issue for follow up CSS improvisations

ok 👍

@kushthedude kushthedude merged commit 55c52ea into fossasia:development Jan 24, 2020
@maze-runnar maze-runnar deleted the patch-5 branch January 24, 2020 10:51
@snitin315 snitin315 mentioned this pull request Feb 9, 2020
Closed
@snitin315 snitin315 mentioned this pull request Feb 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kushthedude kushthedude kushthedude approved these changes

@iamareebjamal iamareebjamal iamareebjamal left review comments

@uds5501 uds5501 Awaiting requested review from uds5501

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automatic scaling feature for speakers photos
3 participants
@maze-runnar @kushthedude @iamareebjamal