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: Fix the last avatar being truncated in members component #491

Merged
merged 1 commit into from
Oct 17, 2017
Merged

fix: Fix the last avatar being truncated in members component #491

merged 1 commit into from
Oct 17, 2017

Conversation

machour
Copy link
Member

@machour machour commented Oct 17, 2017

Also extracted the avatar size into a variable

@chinesedfan
Copy link
Member

@machour Good job! But I am not sure whether it is fixed in different kinds of screen width devices.

@machour
Copy link
Member Author

machour commented Oct 17, 2017

@chinesedfan What resolution do you think can be problematic here? I'm simply using a margin on the list instead of the padding that was set.

Here's how it is in the inspector now:

screen shot 2017-10-17 at 3 48 05 pm

@chinesedfan
Copy link
Member

chinesedfan commented Oct 17, 2017

@machour We have avatarWidth = 30 and avatarMarginRight = 5, so that the width of 10 members will be 30 * 10 + 5 * (10 - 1) = 345. After added your paddings, 2 * 15, it happens to equal to the iPhone6 screen width, 375.

It is only perfect for devices with screen width like 375, 340, 305, and so on. For example, in iPhone5, whose screen width is 320, the last avatar only has 15/30 is shown.

@machour
Copy link
Member Author

machour commented Oct 17, 2017

Oh, I get what you're saying.
Personally, as this is a scrolling list, I think that it's better if we have an avatar not displayed fully at the end, so the user understands that he can scroll.

Prior to my PR, the last avatar, when we scrolled to the end of the list, was cropped (check on your master checkout to see the bug)

@machour
Copy link
Member Author

machour commented Oct 17, 2017

To avoid confusion, this is what this PR fixes:

before: (last user is cropped)
screen shot 2017-10-17 at 4 28 00 pm

after:
screen shot 2017-10-17 at 4 25 19 pm

Copy link
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Beautiful thank you 🙌

@housseindjirdeh housseindjirdeh merged commit b6e2941 into gitpoint:master Oct 17, 2017
@machour machour deleted the fix-members-list branch October 17, 2017 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants