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

Updated LHN layout in GSD Priority Mode #2015

Merged
merged 7 commits into from
Mar 26, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Mar 23, 2021

Please review @shawnborton.

Fixed Issues

Fixes #1914

Tests

  1. Toggle PriorityMode to GSD from Profile preferences and observe the layout change on all platforms.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

image

Desktop

image

Android

1616614230328

@shawnborton
Copy link
Contributor

A couple of pieces of design guidance before I dive in:

The new size of the avatar area should be 28x28:
image

image

For multiple avatars, each one should be 18x18:
image

The border width around the bottom right avatar should be 3:
image

And notice that the font size inside of that bottom right bubble is 9 - we'll have to create a new fontSize variable for that:
image

The gap between the avatar and the grouping of the username/preview is 12:
image

The gap between the username and the message preview is 8:
image

@parasharrajat
Copy link
Member Author

Thanks for the info @shawnborton. Matter of fact, I chose the same size as you mentioned. But still needs to adjust a few. Please review it when I take it out of the draft. Thanks.

@shawnborton
Copy link
Contributor

Sounds good, just ping me when it's ready :)

@parasharrajat
Copy link
Member Author

Updated @shawnborton.

@parasharrajat parasharrajat marked this pull request as ready for review March 23, 2021 19:04
@parasharrajat parasharrajat requested a review from a team as a code owner March 23, 2021 19:04
@botify botify requested review from ctkochan22 and removed request for a team March 23, 2021 19:04
@shawnborton
Copy link
Contributor

Looking good! One small thing that I notice is that the outer border of the bottom right avatar in the avatar group looks strange:
image

I think this is because the border radius is not large enough, and thus we aren't getting a perfect circle for this outer border. I think this problem might actually already exist today. Anyways, I think if we just bump up the border radius to being something larger than just avatarSize/2, we should be good. Let me know if that makes sense!

@parasharrajat
Copy link
Member Author

I think why it is like that.
image

@shawnborton
Copy link
Contributor

Ah interesting. Well that is a strange way to implement that. In the very least, that red portion should be larger.

ctkochan22
ctkochan22 previously approved these changes Mar 23, 2021
@ctkochan22
Copy link
Contributor

@shawnborton Code looks ok. Are we still waiting on the change to make the red portion larger?

I'll default to you for final approval and merge

@shawnborton
Copy link
Contributor

@parasharrajat let me know if you will be addressing the border issue mentioned above.

@parasharrajat
Copy link
Member Author

Yes. I am working on it

@parasharrajat
Copy link
Member Author

parasharrajat commented Mar 24, 2021

@shawnborton New border implementation.
borderWidth is 3 for both modes(GSD and Normal)

Web Android
image Screenshot_2021-03-24-17-56-16-964_com expensify chat

@shawnborton
Copy link
Contributor

Tested and this looks great! I know you didn't introduce this, but one thing I think we could benefit from is to remove the right padding from the wrapper that wraps the avatar + name + preview:

image

Change that to:
image

That will give us a little bit of extra room for the message previews to show. Thoughts on that?

@parasharrajat
Copy link
Member Author

parasharrajat commented Mar 24, 2021

Yeah I like that. Should I do this only for GSD mode layout?

Now I see one issue as well.

When we hover the row the background border on the multiple Avatars in not adopting the colour. I need to fix that as well.

@shawnborton
Copy link
Contributor

Yeah I like that. Should I do this only for GSD mode layout?

I think for both modes is okay.

@parasharrajat
Copy link
Member Author

parasharrajat commented Mar 24, 2021

@shawnborton Updated and Fixed the issue as well. Now working well.

BorderRadius

image

Hover Style (Notice the Border around the second avatar when we hover)

image

@shawnborton
Copy link
Contributor

Cool, thanks for updating. I think you just need to update the screenshots in the original comment above too.

@parasharrajat
Copy link
Member Author

Screenshots updated. @shawnborton I think it can be merged now. #2015 (comment)

@shawnborton
Copy link
Contributor

@ctkochan22 all you!

@ctkochan22 ctkochan22 merged commit d3421ad into Expensify:master Mar 26, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Spotted a few things I think we should clean up here.

@@ -107,20 +128,27 @@ const OptionRow = ({
<MultipleAvatars
avatarImageURLs={option.icons}
optionIsFocused={optionIsFocused}
size={mode === 'compact' ? 'small' : 'default'}
styles={hovered && !optionIsFocused && {
secondAvatar: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in styles.js not right in the JSX I think?

src/components/MultipleAvatars.js Show resolved Hide resolved
src/components/Avatar.js Show resolved Hide resolved
@@ -1,7 +1,7 @@
import React, {memo} from 'react';
import PropTypes from 'prop-types';
import {Image, Text, View} from 'react-native';
import styles from '../styles/styles';
import globalStyles from '../styles/styles';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really into the globalStyles convention. Can we just keep this as styles and then if there are some more specific styles passed in as props give them a more descriptive name? Usually the prop styles have a specific purpose.

avatarImageURLs, optionIsFocused, size, styles,
}) => {
const avatarContainerStyles = [
size === 'small' ? globalStyles.emptyAvatarSmall : globalStyles.emptyAvatar, styles.emptyAvatar,
Copy link
Contributor

Choose a reason for hiding this comment

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

styles.emptyAvatar should go on a new line to make this more readable

src/components/MultipleAvatars.js Show resolved Hide resolved
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.

Update LHN layout in GSD Priority Mode
5 participants