[HOLD for payment 2023-07-17] [$2000] Feature Request: Update header for 6 person DMs to show all avatars

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Problem:

DM members in group chats are capped at a maximum of 4 avatars

Solution:

DMs are capped at 6 members and then add +1 to show more members, Once we get to 5, we use a smaller avatar size

Context/Examples/Screenshots/Notes:

Logs: https://stackoverflow.com/c/expensify/questions/4856

Expensify/Expensify Issue URL: Issue reported by:@puneetlath Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1678715475290659

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c018b3452ff97bbe
Upwork Job ID: 1635771179154558976
Last Price Increase: 2023-06-16

MelvinBot commented 1 year ago

Triggered auto assignment to @mateocole (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[x] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[x] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

MelvinBot commented 1 year ago

Triggered auto assignment to @shawnborton (Design), see these Stack Overflow questions for more details.

shawnborton commented 1 year ago

Looks good to me!

mateocole commented 1 year ago

@shawnborton is this something we will likely be looking at updating internally?

puneetlath commented 1 year ago

This can be done by external contributors.

mateocole commented 1 year ago

got it, in that case going to mark as external

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01c018b3452ff97bbe

MelvinBot commented 1 year ago

Current assignee @mateocole is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

MelvinBot commented 1 year ago

Triggered auto assignment to @NikkiWines (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

allroundexperts commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Avatar header in the report chat is limited to 4 avatars.

What is the root cause of that problem?

We've set CONST.REPORT.MAX_PREVIEW_AVATARS to 4.

What changes do you think we should make in order to solve the problem?

We can change CONST.REPORT.MAX_PREVIEW_AVATARS to 6. After that, we can add a condition here like:

    const isAvatarLargeSize = props.icons.length <= 4;

Then in the same file, we can update the iconStyle variable such that it becomes:

const iconStyle = [
        styles.roomHeaderAvatar,

        // Due to border-box box-sizing, the Avatars have to be larger when bordered to visually match size with non-bordered Avatars
        StyleUtils.getAvatarStyle(isAvatarLargeSize ? CONST.AVATAR_SIZE.LARGE_BORDERED : CONST.AVATAR_SIZE.MEDIUM_BORDERED),
    ];

After this, we can change the size prop of the Avatar component in the same file to be:

  size={isAvatarLargeSize ? CONST.AVATAR_SIZE.LARGE : CONST.AVATAR_SIZE.MEDIUM}

We'll also have to change StyleUtils.getAvatarBorderRadius(CONST.AVATAR_SIZE.LARGE_BORDERED, icon.type) to be:

StyleUtils.getAvatarBorderRadius(isAvatarLargeSize ? CONST.AVATAR_SIZE.LARGE_BORDERED : CONST.AVATAR_SIZE.MEDIUM_BORDERED, icon.type)

This will cause the avatar to show in medium size if there are more than 4 of them. As such, 6 of them can be shown easily.

Note: We have to create a new const called CONST.AVATAR_SIZE.MEDIUM_BORDERED and set it's value to 60 (because CONST.AVATAR_SIZE.MEDIUM is 52 right now). We'll also have to update the StyleUtils.getAvatarBorderRadius function such that it handles the CONST.AVATAR_SIZE.MEDIUM_BORDERED variable correctly.

I've used 4 avatars as a break point between large and small avatars.

@shawnborton can confirm the exact values for the border, avatar size and the number of avatars that cause the breakpoint.

To improve the code further, we can also create a utility function called StyleUtils.getReportHeaderAvatarSize which takes input the number of avatars and returns the correct size. There are numerous ways to do the changes above. The exact code that we use can be decided upon in the PR review.

Result

What alternative solutions did you explore? (Optional)

None

Prince-Mendiratta commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

More of a feature request than a bug.

What is the root cause of that problem?

As of now, the maximum possible participants that can be created a group with is 8. This is a small number and we can adjust the avatar sizes to have all the participant's avatar show up in the ReportActionItemCreated component. Currently, we have set the amount of participants before the +1 mask is shown to be 4.

What changes do you think we should make in order to solve the problem?

Firstly, would like to point out that I reached out to the team on slack to report and confirm behaviour on this issue, since the maximum allowed members are 8, not 6.

The fix will involve the following steps:

In CONST.js, we will have to set the value of MAX_PREVIEW_AVATARS to 8.

In RoomHeaderAvatars, we will introduce 2 new variables,

const borderSize = props.icons.length <= 4 ? CONST.AVATAR_SIZE.LARGE_BORDERED : CONST.AVATAR_SIZE.MEDIUM_BORDERED;
const avatarSize = props.icons.length <= 4 ? CONST.AVATAR_SIZE.LARGE : CONST.AVATAR_SIZE.MEDIUM;

We will modify all the occurrences of CONST.AVATAR_SIZE to reference to the newly declared variables in these places: https://github.com/Expensify/App/blob/ad1e244cacc5ae96f70f6fc42f2b0090efb9b294/src/components/RoomHeaderAvatars.js#L45 https://github.com/Expensify/App/blob/ad1e244cacc5ae96f70f6fc42f2b0090efb9b294/src/components/RoomHeaderAvatars.js#L55 https://github.com/Expensify/App/blob/ad1e244cacc5ae96f70f6fc42f2b0090efb9b294/src/components/RoomHeaderAvatars.js#L58 https://github.com/Expensify/App/blob/ad1e244cacc5ae96f70f6fc42f2b0090efb9b294/src/components/RoomHeaderAvatars.js#L70
In CONST.js, we will add a new MEDIUM_BORDERED key to the AVATAR_SIZE property with value medium-bordered.
We will have to add the value of the new key in CONST.js to these places, whilst only avatarBorderSizes is necessary, we can add avatarSizes as well for consistency. https://github.com/Expensify/App/blob/ad1e244cacc5ae96f70f6fc42f2b0090efb9b294/src/styles/StyleUtils.js#L32 https://github.com/Expensify/App/blob/ad1e244cacc5ae96f70f6fc42f2b0090efb9b294/src/styles/StyleUtils.js#L44
Considering that the gap between avatarSizeLarge and avatarSizeLargeBordered is of 8, we can have the avatarSizeMediumBordered to be of 60 since avatarSizeMedium is 52.

What alternative solutions did you explore? (Optional)

We could also possibly reduce the avatar size only for native devices since the large icons seem to fit pretty well on browsers? Will need for input from @shawnborton for this to explore this further though.

abdulrahuman5196 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Feature request to show 6 person Avatars in chat

What is the root cause of that problem?

Currently we are using room header component which is limited to 4 avatars being shown. It has following disadvantages to have this feature request working

It is hardcoded to show 4 icons (CONST.REPORT.MAX_PREVIEW_AVATARS)
The styles are hardcoded to show large icon sizes. (CONST.AVATAR_SIZE.LARGE)
Same goes for all other style factors like '+1' text style

What changes do you think we should make in order to solve the problem?

Background We have already made MultipleAvatars Component generic to support multiple sizes of avatar dynamically as part of this issue - https://github.com/Expensify/App/issues/15083#issuecomment-1441221893, https://github.com/Expensify/App/issues/15083#issuecomment-1455196772 The changes are in PR review state - https://github.com/Expensify/App/pull/15672 This supports

Dynamically resizing based on the avatar sizes provided like SMALL, LARGE, LARGE_BORDERED, etc.
Show related border width based on the avatar
Shows +1 font size based on the avatar size
And other style changes to make it generic

Changes required

We should replace the use of RoomHeaderAvatar to MultipleAvatar component in ReportActionItemCreated, this way we can show different avatar sizes based on number icons to show or screen size. For eg. On phones when we have greater than 5/6 users, we can choose to show SMALL avatar and when we less users we can choose to show LARGE avatar size. I am just saying this as example, we can choose any of the avatar sizes for this. Or we can also choose to show one avatar size for all cases as well
We should expose a param like 'maxAvatarToShow' in MultipleAvatar component for which in this case we can provide 6 so that we will display 6 avatars and show +1 after that if required. We will also have to update logic in MultipleAvatars to support number of avatars to show based on this external param in the MultipleAvatars component
I can see the width of one avatar overlapping with other is less in this chat page mock compared to the invite page mock, so we can expose a param like 'overlapSize' to provide the size on avatar should overlap on other. And we should add the logic for MultipleAvatars component use the overlapSize from params.(Only if this is required) The code changes for both the above changes need to be done here - https://github.com/Expensify/App/blob/5cba7218adbc0d4f8383b312beb7b9d90d2f8898/src/components/MultipleAvatars.js#L89-L174 to update the use of dynamic params from components using MultipleAvatars (I am not adding exact code diffs as its against current guidelines) Optional: RoomHeaderAvatar is also used on ReportDetailsPage as well, we can replace there as well if required.

@shawnborton On high level we can re-use the MultipleAvatars component made in a generic way as part of the Invite Message page(https://github.com/Expensify/App/issues/15083) for this issue as well with some modifications as mentioned above

What alternative solutions did you explore? (Optional)

NA

Snehal-Techforce commented 1 year ago

Proposal

Problem: DM members in group chats are capped at a maximum of 4 avatars

Solution:

MAX_PREVIEW_AVATARS constant value needs to change from 4 to 6 in CONST.js file.

CSS change would be required in RoomHeaderAvatars.js file as below.

Expected Output:

For 6 avatars it will be displayed as below:
For 6+ avatars , it will be displayed as below:
For <6 avatars , it will be displayed as below:

MelvinBot commented 1 year ago

📣 @Snehal-Techforce! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

mananjadhav commented 1 year ago

Going to review proposals in a few hours.

Snehal-Techforce commented 1 year ago

Contributor details Your Expensify account email: reach@techforceglobal.com Upwork Profile Link: https://www.upwork.com/freelancers/~01fe25025615ae221e?viewMode=1

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

NikkiWines commented 1 year ago

Going to be OOO next week so going to reapply the External label so that I don't hold things up!

MelvinBot commented 1 year ago

Current assignee @mateocole is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Current assignee @mananjadhav is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Triggered auto assignment to @roryabraham (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

mananjadhav commented 1 year ago

Apologies for the delay, I was OOO. I'll check the proposals today. I did go through them once and I am checking if @abdulrahuman5196's proposal of making MultipleAvatars work would be beneficial.

@Snehal-Techforce quick feedback, please don't add Code diffs in the proposals. They would ignored from the review, we're focused on explaining the technical details of the proposals, along with some pseudocode may be.

Snehal-Techforce commented 1 year ago

Thank you for your comment and providing feedback as well @mananjadhav. For upcoming proposals, I will not add Code diffs.

MelvinBot commented 1 year ago

Upwork job price has been updated to $1000

roryabraham commented 1 year ago

I like @abdulrahuman5196's proposal to re-use the MultipleAvatars component and get rid of RoomHeadersAvatar. I want to add a few additional requirements:

Must get rid of other instances of RoomHeadersAvatar and standardize on MultipleAvatars
Should support up to 7 avatars (the maximum for a group chat)
Should only use smaller icons if the screen is narrow – on wider screens should continue to use the current avatar size.
Storybook stories for the MultipleAvatars component

MelvinBot commented 1 year ago

📣 @abdulrahuman5196 You have been assigned to this job by @roryabraham! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

mananjadhav commented 1 year ago

I think we should put this on HOLD until https://github.com/Expensify/App/pull/15672 is merged but I liked @abdulrahuman5196 suggestion of using MultipleAvatars component.

mananjadhav commented 1 year ago

Ohh @roryabraham I just saw your comment, what do you think of putting this on Hold until the linked issue is merged as it is still WIP and will conflict with the current design. Agreed with the additional requirements though.

roryabraham commented 1 year ago

HOLDing on https://github.com/Expensify/App/pull/15672

allroundexperts commented 1 year ago

I like @abdulrahuman5196's proposal to re-use the MultipleAvatars component and get rid of RoomHeadersAvatar. I want to add a few additional requirements:

Must get rid of other instances of RoomHeadersAvatar and standardize on MultipleAvatars

Should support up to 7 avatars (the maximum for a group chat)

Should only use smaller icons if the screen is narrow – on wider screens should continue to use the current avatar size.

Storybook stories for the MultipleAvatars component

Thank you for the review @roryabraham!

I was wondering why we're stacking different use cases inside of a single component? Another place where I noticed this pattern was in the Modal component which stacks both Popover and Modal inside of a single Modal component. While this approach surely has its own benefits, in the long term, don't you think that we would end up having too many use cases inside a single place separated by conditionals? Down the road, lets say we want to support another size of avatars in MultipleAvatars. We'll have to add another prop that will conditionally render a different size. Doing so again and again, will just make the component more fragile. IMO, we should either make use of composition (if we want to support a single component having multiple variants) or have a straight away flat component hierarchy where each variant is contained in its own component. Please let me know your thoughts.

abdulrahuman5196 commented 1 year ago

I like @abdulrahuman5196's proposal to re-use the MultipleAvatars component and get rid of RoomHeadersAvatar. I want to add a few additional requirements:
1. Must get rid of other instances of `RoomHeadersAvatar` and standardize on `MultipleAvatars`

2. Should support up to 7 avatars (the maximum for a group chat)

3. Should only use smaller icons if the screen is narrow – on wider screens should continue to use the current avatar size.

4. Storybook stories for the `MultipleAvatars` component

@roryabraham Thank your assigning the issue to me. I agree to hold this issue until the refractor PR is merged. We are on final stage of review there and should be closing out the PR soon. After that I will start working on the requirements here.

Wanted to confirm on the below meanwhile. Requests 1,2 and 4 are fine.

Should only use smaller icons if the screen is narrow – on wider screens should continue to use the current avatar size.

On the OP it is mentioned "Once we get to 5, we use a smaller avatar size", then this should only apply to smallScreenWidth cases right? The UI permutation will be,

case 1 : isSmallScreenWidth and 4 avatars = AVATAR_SIZE.LARGE case 2: isSmallScreenWidth and above 4 avatars = AVATAR_SIZE.SMALL case 3: not isSmallScreenWidth and 4 avatars = AVATAR_SIZE.LARGE case 3: not isSmallScreenWidth and above 4 avatars = AVATAR_SIZE.LARGE Correct me if any of the above case needs to be changed.

roryabraham commented 1 year ago

@abdulrahuman5196 yep, that looks correct 👍🏼

mateocole commented 1 year ago

Going to be OOO, reassigning this

roryabraham commented 1 year ago

Moving this to weekly while it's on HOLD

shawnborton commented 1 year ago

Not overdue

slafortune commented 1 year ago

Still on hold for https://github.com/Expensify/App/pull/15672

abdulrahuman5196 commented 1 year ago

The PR https://github.com/Expensify/App/pull/15672 has been merged. Just to confirm, before I start working on this feature request since it has been couple of weeks before this issue was put on hold.

1) This is the new invite members screen using the new multiple avatars component. We are expecting the same with the support to show all avatars for big screen and for small screen with over 4 avatars we choose smaller size. 2) Remove RoomHeadersAvatar usages and replace it with MultipleAvatars. 3) Create storybook for multiple avatars @roryabraham @mananjadhav

puneetlath commented 1 year ago

Yes, that sounds about right to me. Main thing is that, given that you can have at most 8 people in a group DM, we should make it so that all the avatars fit in the room header in all scenarios.

mananjadhav commented 1 year ago

make it so that all the avatars fit in the room header in all scenarios.

@puneetlath I didn't get you mean by this. Can you elaborate?

abdulrahuman5196 commented 1 year ago

I assume this is the permutation @mananjadhav

On the OP it is mentioned "Once we get to 5, we use a smaller avatar size", then this should only apply to smallScreenWidth cases right? The UI permutation will be, case 1 : isSmallScreenWidth and 4 avatars = AVATAR_SIZE.LARGE case 2: isSmallScreenWidth and above 4 avatars = AVATAR_SIZE.SMALL case 3: not isSmallScreenWidth and 4 avatars = AVATAR_SIZE.LARGE case 3: not isSmallScreenWidth and above 4 avatars = AVATAR_SIZE.LARGE

as mentioned here - https://github.com/Expensify/App/issues/15929#issuecomment-1483766334

Correct me if i am wrong @puneetlath

puneetlath commented 1 year ago

@puneetlath I didn't get you mean by this. Can you elaborate?

I just meant that no matter the screen size, we should make it so that all 8 avatars fit here. Given that we know that 8 is the max there will be.

mananjadhav commented 1 year ago

Cool. Thanks for clarifying. I meant for the smaller screen, it should auto adjust. Understood from the previous comments.

abdulrahuman5196 commented 1 year ago

@puneetlath @mananjadhav @roryabraham

I just meant that no matter the screen size, we should make it so that all 8 avatars fit here. Given that we know that 8 is the max there will be.

How much smaller screens are we thinking to support? There has to be a min supported width below which we won't support(If not we have to reduce the avatar size to be shown accordingly)

I am adding the permutations based on sizes I tested from the OP screenshot and their approx worst case width requirement for the MultipleAvatars component including all avatars to be rendered. Based on the permutations I provided during proposal

Case 1: isSmallScreenWidth and 4 avatars = AVATAR_SIZE.LARGE(80px one avatar) - 240px(approx) min width required(excluding paddings) Case 2: isSmallScreenWidth and above 4 avatars = AVATAR_SIZE.MEDIUM(52px one avatar) - 312px(approx) min width required Case 3: not isSmallScreenWidth and 4 avatars = AVATAR_SIZE.LARGE - 240px(approx) min width required Case 4: not isSmallScreenWidth and above 4 avatars = AVATAR_SIZE.LARGE - 480px(approx) min width required

Note: If the screen size is below the required min width, all the avatars won't be shown and we might see avatars getting clipped.

From the calculation, I don't think case 3 and 4 will have any issue since its big screen with 800px >= width. Only case we have to think of 1 and 2. If we are expecting to support screen sizes less than 350px, we can reduce the avatar size from medium to small for case 2 or add another permutation

If we could agree on the expected behaviour of how small we want to support I can provide better information. Mostly these are design decisions, so someone from design team should be able to help out as well if required.

Reference:

https://github.com/Expensify/App/blob/8c9e258f2543b6c454a42df82f259c2a8c73b1ed/src/styles/StyleUtils.js#L45-L55

https://github.com/Expensify/App/blob/8c9e258f2543b6c454a42df82f259c2a8c73b1ed/src/styles/variables.js#L31-L41

abdulrahuman5196 commented 1 year ago

Gentle ping on the above. @puneetlath @roryabraham @mananjadhav

It's blocking me to start on the implementation since it's a requirement confirmation.

puneetlath commented 1 year ago

@shawnborton do you have a sense of what the smallest screen size is that we should "reasonably" support?

shawnborton commented 1 year ago

Can you catch me up on what the goal of this issue is? is it to always show all of the avatars of the group chat members, no matter the screen size?

If that's the case we might need to rethink of how this would work on mobile. For example, I'm not sure that we have the width to support 7 avatars on mobile, but we could try.

In terms of minimum screen size, I would think maybe iPhone SE or something like that? Maybe we can make this responsive and if we have the available width, we can show avatars, and if not, we can fall back on something similar to what we currently have.

abdulrahuman5196 commented 1 year ago

@shawnborton

Can you catch me up on what the goal of this issue is? is it to always show all of the avatars of the group chat members, no matter the screen size? If that's the case we might need to rethink of how this would work on mobile. For example, I'm not sure that we have the width to support 7 avatars on mobile, but we could try.

Yes. That's the gist of the issue. Issue creator can correct me if wrong. One correction is we need to support 8 avatars.

In terms of minimum screen size, I would think maybe iPhone SE or something like that? Maybe we can make this responsive and if we have the available width, we can show avatars, and if not, we can fall back on something similar to what we currently have.

Even in the current behaviour it seems to break for iPhone SE first generation size of small phone

Simulator Screen Shot - Iphone SE test - 2023-05-08 at 19 09 44

I tried a quick code to show the expected behaviour For the new proposal. Case 1: isSmallScreenWidth and 4 avatars = AVATAR_SIZE.LARGE, This seems to work fine

Simulator Screen Shot - Iphone SE test - 2023-05-08 at 19 11 06

Case 2: isSmallScreenWidth and above 4 avatars = AVATAR_SIZE.MEDIUM(52px one avatar) Here AVATAR_SIZE.MEDIUM(52px one avatar) seems to be breaking so I choose AVATAR_SIZE.DEFAULT(40px) which is working fine.

Simulator Screen Shot - Iphone SE test - 2023-05-08 at 19 07 36

Furthermore we can dynamically reduce sizes of avatar based on screen sizes to add to the above 4 cases but it might be ideal experience to have more sizes involved.

I think if we want to consider Iphone SE 1st generation phone to be a min supported phone width(which is not supported currently even), then we are good to go IMO.

shawnborton commented 1 year ago

@puneetlath curious for your thoughts here since I believe this was your idea re: the empty state avatars.

I think i would prefer to keep them large, and just show as many as we can based on screen width.

Expensify / App