Web - Expense request avatar is broken in LHN

kbecciv 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!

Action Performed:

Login account A with policyExpenseChat beta permission
Create workspace if not exist
Invite user B to workspace
Login user B on another device
On B, Go to invited A's workspace chat
Request money
click ReportPreview to go to expense report
click IOUPreview to go to expense request
Observe highlighted row in LHN

Expected Result:

Avatar is not broken

Actual Result:

Avatar is broken

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.47-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/8086ed37-55a7-4d0b-995c-a8d6ee3ea441

https://github.com/Expensify/App/assets/93399543/4a252a3c-cba3-4243-bf24-6a0413089d74

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

View all open jobs on GitHub

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] 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
[ ] 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.
[ ] 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.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

bernhardoj commented 1 year ago

Proposal

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

The expense report first avatar is not full circle.

What is the root cause of that problem?

The first avatar/icon size is 32, but the width of the container of the avatar is 40 and the border-radius style is applied to the container.

The width of 40 comes from the containerStyle here that is applied to the SubscriptAvatar container. https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/components/SubscriptAvatar.js#L51

and because the default flex-direction is column, the children, including the avatar container, will have the same width, that is 40.

That's why you can notice the border-top radius looking weird

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

Apply alignSelf flex start to the first avatar container of SubscriptAvatar, so the width follows the content/children of it. https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/components/SubscriptAvatar.js#L56-L63

What alternative solutions did you explore? (Optional)

If we look at MultipleAvatars that is stacked diagonally,

it doesn't have this issue. That is because we manually set the width and the height of the container (imageStyles) to be the same size as the avatar icon. (just a note: avatar has a total of 3 view wrappers that we can style: iconAdditionalStyles, imageStyles, containerStyles) https://github.com/Expensify/App/blob/eb020063789091eb2e3086b5b54af3ead8436645/src/components/MultipleAvatars.js#L253-L262

We can do the similar thing, but I still prefer the main solution.

ahmedGaber93 commented 1 year ago

@situchan How can I get account with policyExpenseChat beta permission? I was overriding the Permissions.canUseAllBetas function to return true in my local, but when opening any workspace chat it return 304. Do I need to have this permission in backend also? cc @bernhardoj

bernhardoj commented 1 year ago

@ahmedGaber93 yes

ahmedGaber93 commented 1 year ago

@bernhardoj Thanks for help. But as a contributor, how can I get this permission in backend?

bernhardoj commented 1 year ago

You can ask for help from the internal team to add you to the beta.

situchan commented 1 year ago

@ahmedGaber93 please request permission in slack. You need to provide emails to be added to beta.

ahmedGaber93 commented 1 year ago

Thanks for help @situchan. I have request permission in slack.

situchan commented 1 year ago

@stephanieelliott can I C+ review this if possible? As I found the root cause and solution while reporting this bug.

Christinadobrzyn commented 1 year ago

This issue seems similar to https://github.com/Expensify/App/issues/23847 - it looks like it's going to be fixed with https://github.com/Expensify/App/pull/23815 - I asked the engineers in 23815 if they agree

situchan commented 1 year ago

This issue seems similar to #23847 - it looks like it's going to be fixed with #23815 - I asked the engineers in 23815 if they agree

@Christinadobrzyn No, it's different issue. #23815 was deployed to staging 6 hrs ago but broken avatar issue is still reproducible.

rezkiy37 commented 1 year ago

This issue seems similar to #23847 - it looks like it's going to be fixed with #23815 - I asked the engineers in 23815 if they agree

FYI: https://github.com/Expensify/App/pull/23815#issuecomment-1660110273

Christinadobrzyn commented 1 year ago

thanks for confirming @rezkiy37! Closing this as it's confirmed to be fixed with https://github.com/Expensify/App/pull/23815#issuecomment-1660110273

@situchan feel free to reach out in the Expensify-Bugs room if you find any regressions

Christinadobrzyn commented 1 year ago

Hey @rezkiy37, can you double-check this issue will be fixed? @situchan believes to think the issue still exists after the merge.

Sorry for all the confusion @stephanieelliott - tried to save you some time!

rezkiy37 commented 1 year ago

Hi, @situchan! Could you please take a look at this comment where I attached examples. The second video associated with this issue. Do I still have the bug there?

https://github.com/Expensify/App/pull/23815#issuecomment-1660110273

situchan commented 1 year ago

@rezkiy37 you can test on staging right now

situchan commented 1 year ago

@rezkiy37 I checked your video. It's IOU request (in DM). The issue is in expense request (in workspace chat). Is it clear now?