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

[HOLD for payment 2024-10-16] [$250] Increase beginningOfChat avatar from 80px to 100px #49435

Closed
dubielzyk-expensify opened this issue Sep 19, 2024 · 23 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@dubielzyk-expensify
Copy link
Contributor

dubielzyk-expensify commented Sep 19, 2024

We've recently decided to simplify the avatar sizes across the app and as a result we want to update the beginningOfChat avatar size from 80px to 100px. Here's what we want:

image

image

Nothing else should change.

cc @Expensify/design

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836877432130950062
  • Upwork Job ID: 1836877432130950062
  • Last Price Increase: 2024-09-19
  • Automatic offers:
    • BhuvaneshPatil | Contributor | 104055874
Issue OwnerCurrent Issue Owner: @puneetlath
@dubielzyk-expensify dubielzyk-expensify added Weekly KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 19, 2024
@shawnborton
Copy link
Contributor

Nice, thanks for spinning up the issue!

@BhuvaneshPatil
Copy link
Contributor

BhuvaneshPatil commented Sep 19, 2024

Edited by proposal-police: This proposal was edited at 2024-09-19 04:14:41 UTC.

Proposal

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

Increase beginningOfChat avatar from 80px to 100px

What is the root cause of that problem?

We use LARGE size.

<MultipleAvatars
icons={icons}
size={isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.LARGE : CONST.AVATAR_SIZE.MEDIUM}
shouldStackHorizontally
shouldDisplayAvatarsInRows={shouldUseNarrowLayout}
maxAvatarsInRow={shouldUseNarrowLayout ? CONST.AVATAR_ROW_SIZE.DEFAULT : CONST.AVATAR_ROW_SIZE.LARGE_SCREEN}
/>

it uses 80px as height and width

[CONST.AVATAR_SIZE.LARGE]: variables.avatarSizeLarge,

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

We need to modify this size, but if we only want to change the size for this particular avatar, we can introduce new key and give it size of 100px.

We can use XLARGE key.

size={isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.XLARGE : CONST.AVATAR_SIZE.MEDIUM}

If we want to keep XLARGE for all screens

size={CONST.AVATAR_SIZE.XLARGE}

What alternative solutions did you explore? (Optional)

@BhuvaneshPatil
Copy link
Contributor

@dubielzyk-expensify @shawnborton , In case of multiple avatar (group chat), do we need to change the size as well? or only for single avatar

@nkdengineer
Copy link
Contributor

nkdengineer commented Sep 19, 2024

Proposal

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

Increase beginningOfChat avatar from 80px to 100px

What is the root cause of that problem?

Currently we use avatar size is LARGE here

size={isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.LARGE : CONST.AVATAR_SIZE.MEDIUM}

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

Simply we should change the LARGE size in here to XLARGE size

size={CONST.AVATAR_SIZE.XLARGE}

size={isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.LARGE : CONST.AVATAR_SIZE.MEDIUM}

What alternative solutions did you explore? (Optional)

@nyomanjyotisa
Copy link
Contributor

Proposal

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

Increase beginningOfChat avatar from 80px to 100px

What is the root cause of that problem?

We use large and medium here

size={isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.LARGE : CONST.AVATAR_SIZE.MEDIUM}

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

If we only want to change the size to 100px if isLargeScreenWidth or (icons && icons.length < 3) we can change to the following

size={isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.XLARGE : CONST.AVATAR_SIZE.MEDIUM}

What alternative solutions did you explore? (Optional)

If we want to change the size to 100px on all conditions, we can change to the following

size={CONST.AVATAR_SIZE.XLARGE}

If we want to change the size to 100px if isLargeScreenWidth or (icons && icons.length < 3) and otherwise set the size to 80px, we can change to the following

size={isLargeScreenWidth || (icons && icons.length < 3) ? CONST.AVATAR_SIZE.XLARGE : CONST.AVATAR_SIZE.LARGE}

@dubielzyk-expensify
Copy link
Contributor Author

@dubielzyk-expensify @shawnborton , In case of multiple avatar (group chat), do we need to change the size as well? or only for single avatar

Yes, though I believe group chat actually just creates a group avatar now. But yes, all avatars that's in the start of chat area should be 100x100:

image

@shawnborton
Copy link
Contributor

Yup exactly, new group chats would only have the one group chat avatar, not multiple like they used to. No idea how this impacts older group chats that were created but that won't effect new users on NewDot.

@nkdengineer
Copy link
Contributor

update proposal following the new requirement here

@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

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

@melvin-bot melvin-bot bot changed the title Increase beginningOfChat avatar from 80px to 100px [$250] Increase beginningOfChat avatar from 80px to 100px Sep 19, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

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

@eh2077
Copy link
Contributor

eh2077 commented Sep 20, 2024

Thanks for your proposals!

This one is relatively straightforward. So I think we can go ahead with @BhuvaneshPatil 's proposal because their proposal was first and they started a discussion to address ambiguity about group chats.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 20, 2024

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

📣 @BhuvaneshPatil 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 20, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Oct 9, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production labels Oct 9, 2024
@melvin-bot melvin-bot bot changed the title [$250] Increase beginningOfChat avatar from 80px to 100px [HOLD for payment 2024-10-16] [$250] Increase beginningOfChat avatar from 80px to 100px Oct 9, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 9, 2024
Copy link

melvin-bot bot commented Oct 9, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Oct 9, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.46-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-16. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Oct 9, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@eh2077] The PR that introduced the bug has been identified. Link to the PR:
  • [@eh2077] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@eh2077] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@eh2077] Determine if we should create a regression test for this bug.
  • [@eh2077] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Oct 16, 2024
Copy link

melvin-bot bot commented Oct 21, 2024

@puneetlath, @BhuvaneshPatil, @eh2077 Eep! 4 days overdue now. Issues have feelings too...

@puneetlath
Copy link
Contributor

@BhuvaneshPatil has been paid.

@eh2077 bump on the checklist!

@melvin-bot melvin-bot bot removed the Overdue label Oct 21, 2024
@eh2077
Copy link
Contributor

eh2077 commented Oct 22, 2024

Checklist

  • [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I don't think this is a bug, so there isn't a PR causes this.
  • [@eh2077] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
  • [@eh2077] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • [@eh2077] Determine if we should create a regression test for this bug. No

@eh2077
Copy link
Contributor

eh2077 commented Oct 22, 2024

Requested payment in NewDot

@puneetlath
Copy link
Contributor

Payment summary:

Thanks everyone!

@JmillsExpensify
Copy link

$250 approved for @eh2077

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants