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

enh: speaker image is showing properly in mobile #3848

Merged
merged 5 commits into from
Jan 25, 2020
Merged

enh: speaker image is showing properly in mobile #3848

merged 5 commits into from
Jan 25, 2020

Conversation

maze-runnar
Copy link
Contributor

Fixes #3847
Screenshot from 2020-01-24 16-45-52
Screenshot from 2020-01-24 16-45-55

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)

snitin315
snitin315 reviewed Jan 24, 2020
View reviewed changes
app/styles/components/speaker-list.scss Outdated
overflow: hidden;
position: relative;
width: 220px;
overflow: hidden;
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

due to eslint warning .

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

@kushthedude kushthedude left a comment •
edited
Loading

Choose a reason for hiding this comment

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

CSS needs to be changed using the object-fit as I stated in the previous PR

@maze-runnar
Copy link
Contributor Author

maze-runnar commented Jan 24, 2020 •
edited
Loading

Screenshot from 2020-01-24 17-13-50
Screenshot from 2020-01-24 17-13-53
reference : https://devdocs.io/css/object-fit

@maze-runnar
Copy link
Contributor Author

CSS needs to be changed using the object-fit as I stated in the previous PR

object-fit : cover works quite well -
After :
Screenshot from 2020-01-24 17-13-53
Before :
Screenshot from 2020-01-24 17-16-50

@kushthedude
Copy link
Member

kushthedude commented Jan 24, 2020 via email

@maze-runnar
Copy link
Contributor Author

Screenshot from 2020-01-24 17-13-50
Screenshot from 2020-01-24 17-13-53
reference : https://devdocs.io/css/object-fit

@kushthedude , they are also working well, this screenshot is also belong to object-fit .

@kushthedude
Copy link
Member

kushthedude commented Jan 24, 2020 via email

@maze-runnar
Copy link
Contributor Author

Check the css work on summit 2020 and see if all speaker images are centering well.
…
On Fri, 24 Jan, 2020, 17:29 Sundaram Dubey, @.***> wrote: [image: Screenshot from 2020-01-24 17-13-50] https://user-images.githubusercontent.com/56407566/73067019-96f3cf80-3ecd-11ea-8682-9992d5829c7b.png [image: Screenshot from 2020-01-24 17-13-53] https://user-images.githubusercontent.com/56407566/73067020-978c6600-3ecd-11ea-914a-0b5a4ecc8364.png reference : https://devdocs.io/css/object-fit @kushthedude https://github.com/kushthedude , they are also working well, this screenshot is also belong to object-fit . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3848?email_source=notifications&email_token=AKQMTLWFFP3SLOPZEFV3Q5LQ7LJ3DA5CNFSM4KLEUU5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ2SKRY#issuecomment-578102599>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLR5RWLLAPSY4KNE2OLQ7LJ3DANCNFSM4KLEUU5A .

actually , i am using default backend api that's why i am not able to check these changes on summit 2020 at the time.

@kushthedude
Copy link
Member

kushthedude commented Jan 24, 2020 via email

@maze-runnar
Copy link
Contributor Author

Change it then. On Fri, 24 Jan, 2020, 17:35 Sundaram Dubey, notifications@github.com wrote:
…
Check the css work on summit 2020 and see if all speaker images are centering well. … <#m_3893667217965247807_> On Fri, 24 Jan, 2020, 17:29 Sundaram Dubey, @.***> wrote: [image: Screenshot from 2020-01-24 17-13-50] https://user-images.githubusercontent.com/56407566/73067019-96f3cf80-3ecd-11ea-8682-9992d5829c7b.png [image: Screenshot from 2020-01-24 17-13-53] https://user-images.githubusercontent.com/56407566/73067020-978c6600-3ecd-11ea-914a-0b5a4ecc8364.png reference : https://devdocs.io/css/object-fit @kushthedude https://github.com/kushthedude https://github.com/kushthedude , they are also working well, this screenshot is also belong to object-fit . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3848 <#3848>?email_source=notifications&email_token=AKQMTLWFFP3SLOPZEFV3Q5LQ7LJ3DA5CNFSM4KLEUU5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ2SKRY#issuecomment-578102599>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLR5RWLLAPSY4KNE2OLQ7LJ3DANCNFSM4KLEUU5A . actually , i am using default backend api that's why i am not able to check these changes on summit 2020 at the time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3848?email_source=notifications&email_token=AKQMTLRE2YHL4Q67EWHI32LQ7LKQJA5CNFSM4KLEUU5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ2SYSI#issuecomment-578104393>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQMTLXTID3UFVNZNGMPNN3Q7LKQJANCNFSM4KLEUU5A .

Screenshot from 2020-01-24 17-43-40
Screenshot from 2020-01-24 17-43-55

@kushthedude every image is properly centered .

kushthedude
kushthedude reviewed Jan 24, 2020
View reviewed changes
app/styles/components/speaker-list.scss Outdated
width: 220px;
height: 300px;
overflow: hidden;
background-position: center center;
Copy link
Member

Choose a reason for hiding this comment

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

Why this for ?

@maze-runnar
Copy link
Contributor Author

Because working correctly ,and from the same link given , and also most recent one. I think these reasons were enough.

@kushthedude
Copy link
Member

Because working correctly

You don't include code which works correctly, You include the code which you know is most optimal and goes correctly with the system. There are 100 ways to code but few of them are only optimal. This is not a College Project or Assignment sort of thing. It is a working product which is deployed on production and getting used by many. Each & Every line should have a meaning why is it there and if not it should not be.

and from the same link given

I told you before that link is more than 5 years old of the time when css3 was not compatible with many browsers.
Please check and if there is any unneeded property remove it.

@maze-runnar
Copy link
Contributor Author

can't remove overflow , otherwise -
Screenshot from 2020-01-24 20-51-43

@maze-runnar
Copy link
Contributor Author

maze-runnar commented Jan 24, 2020 •
edited
Loading

object-fit is handling a lot of thing , working same after removing position .
Screenshot from 2020-01-24 21-09-35

@kushthedude
Copy link
Member

object-fit is handling a lot of thing , working same after removing position .

This means CSS has made a lot of progress in 5 years from that article was published 😝

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.

lint warnings are still there 😞

@kushthedude
Copy link
Member 

styles/components/speaker-list.scss
 19:2  warning  Mixed tabs and spaces  indentation

@maze-runnar
Copy link
Contributor Author

lint warnings are still there

how to know that there is a warning or not if build is successful 😕

@kushthedude
Copy link
Member

kushthedude commented Jan 24, 2020 via email

@kushthedude kushthedude merged commit 398bb21 into fossasia:development Jan 25, 2020
@maze-runnar maze-runnar deleted the patch-5 branch January 25, 2020 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snitin315 snitin315 snitin315 left review comments

@kushthedude kushthedude kushthedude approved these changes

@uds5501 uds5501 Awaiting requested review from uds5501

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

Successfully merging this pull request may close these issues.

speaker page: speaker image is not properly showing in mobile devices
3 participants
@maze-runnar @kushthedude @snitin315