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

[$250] Shortcut - Group chat counter under Shortcut appears differently than LHN and chat header #39052

Closed
6 tasks done
kbecciv opened this issue Mar 27, 2024 · 35 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Mar 27, 2024

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


Version Number: 1.4.57-0
Reproducible in staging?: y
Reproducible in production?: new feature
Issue reported by:

Action Performed:

Precondition:

  • App is in light mode.
  1. Go to staging.new.expensify.com.
  2. Create a group chat with at least three people.
  3. Create a split bill in the group chat.
  4. Go to FAB.

Expected Result:

The group chat counter under Shortcut appears the same as the counter in LHN and chat header.

EDIT: SEE THIS COMMENT TO BETTER UNDERSTAND WHAT IS EXPECTED!!

Actual Result:

The group chat counter under Shortcut is black in light mode, while it is white in LHN and chat header.

Workaround:

n/a

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

bandicam.2024-03-27.14-40-25-425.mp4

image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0159361e054180c63b
  • Upwork Job ID: 1774874377625939968
  • Last Price Increase: 2024-04-08
  • Automatic offers:
    • DrLoopFall | Contributor | 0
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Mar 27, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Mar 27, 2024

Triggered auto assignment to @cristipaval (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@kbecciv
Copy link
Author

kbecciv commented Mar 27, 2024

We think that this bug might be related to #vip-vsb

@DrLoopFall
Copy link
Contributor

DrLoopFall commented Mar 27, 2024

Proposal

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

Group chat counter under Shortcut appears differently than LHN and chat header

What is the root cause of that problem?

We are using a different style for the small avatar, avatarInnerTextSmall, which has color: theme.text, but for large avatar we are using avatarInnerText which has color: theme.textLight

style={[styles.userSelectNone, size === CONST.AVATAR_SIZE.SMALL ? styles.avatarInnerTextSmall : styles.avatarInnerText]}

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

Update avatarInnerText to use same color as avatarInnerTextSmall(theme.text), so the avatar remains consistent in all the places.

avatarInnerText: {
-   color: theme.textLight,
+   color: theme.text,
    fontSize: variables.fontSizeSmall,
    lineHeight: undefined,
    marginLeft: -3,
    textAlign: 'center',
},

@cristipaval cristipaval added Daily KSv2 Hourly KSv2 and removed Hourly KSv2 DeployBlockerCash This issue or pull request should block deployment Daily KSv2 labels Mar 27, 2024
@cristipaval
Copy link
Contributor

Removing the deploy blocker label given that it is a minor UI issue that doesn't break the functionality

@cristipaval
Copy link
Contributor

Tagging @Expensify/design to confirm this is a bug and to clarify the expected specs.

@dannymcclain
Copy link
Contributor

So I think this is a bug, but I think the actual bug is in the LHN and header, not in the Shortcut.

In dark mode, the +X counter uses a background color of icons with a text color of text. But in light mode, the counter uses a background color of icons with #FFF as the text color (white). We don't typically use pure white in the interface, so I'm thinking the counter in the Shortcut is actually the right version (it looks like it's using icons for the background color and text for the text color like in dark mode, but obviously they are the light mode versions of those variables).

cc'ing @shawnborton because he might know what's up!

@shawnborton
Copy link
Contributor

Hmm yeah, that does seem like a bug and I agree with where you landed Danny.

@melvin-bot melvin-bot bot added the Overdue label Apr 1, 2024
@cristipaval
Copy link
Contributor

Thank you, @dannymcclain and @shawnborton! I'll make this one external to get it fixed!

@melvin-bot melvin-bot bot removed the Overdue label Apr 1, 2024
@cristipaval cristipaval added the External Added to denote the issue can be worked on by a contributor label Apr 1, 2024
@melvin-bot melvin-bot bot changed the title Shortcut - Group chat counter under Shortcut appears differently than LHN and chat header [$500] Shortcut - Group chat counter under Shortcut appears differently than LHN and chat header Apr 1, 2024
Copy link

melvin-bot bot commented Apr 1, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 1, 2024
Copy link

melvin-bot bot commented Apr 1, 2024

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

@paultsimura
Copy link
Contributor

paultsimura commented Apr 1, 2024

On staging and main, the group thumbnail looks like this and the issue is no longer relevant:

image

Didn't we deprecate the counter?

@cristipaval cristipaval changed the title [$500] Shortcut - Group chat counter under Shortcut appears differently than LHN and chat header [$250] Shortcut - Group chat counter under Shortcut appears differently than LHN and chat header Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

Upwork job price has been updated to $250

@cristipaval
Copy link
Contributor

cristipaval commented Apr 8, 2024

I'm also making this one 250$, given that it's a trivial UI thing.

@paultsimura
Copy link
Contributor

There is one proposal which requires a review

@thesahindia
Copy link
Member

So I think this is a bug, but I think the actual bug is in the LHN and header, not in the Shortcut.

@dannymcclain, can you confirm the expected behaviour: We want the LHN and header version to look like the one in the Shortcut? We should use theme.text instead of theme.textLight?

@dannymcclain
Copy link
Contributor

We want the LHN and header version to look like the one in the Shortcut?

Yeah that's what I'm thinking. So that would be like this:

image

@thesahindia
Copy link
Member

Thanks @dannymcclain!

@DrLoopFall, please update your proposal.

@DrLoopFall
Copy link
Contributor

Proposal

Updated

cc @thesahindia

@thesahindia
Copy link
Member

@DrLoopFall's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 10, 2024

Current assignee @cristipaval 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 Apr 10, 2024
Copy link

melvin-bot bot commented Apr 10, 2024

📣 @DrLoopFall 🎉 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 📖

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Apr 12, 2024
@DrLoopFall
Copy link
Contributor

@mvtglobally

The issue is reproducible when creating split bills from existing group chats (which uses the old avatar style)

@DrLoopFall
Copy link
Contributor

@thesahindia
The PR #40175 is ready for review.

@DrLoopFall
Copy link
Contributor

Hi @cristipaval,
The payment for this issue is still pending, can you please process it?

@cristipaval cristipaval added the Bug Something is broken. Auto assigns a BugZero manager. label May 1, 2024
Copy link

melvin-bot bot commented May 1, 2024

Triggered auto assignment to @sakluger (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 May 1, 2024
@cristipaval
Copy link
Contributor

@sakluger could you please help with the payments here? 🙏

@sakluger
Copy link
Contributor

sakluger commented May 2, 2024

Summarizing payment on this issue:

Contributor: @DrLoopFall $250, paid via Upwork
Contributor+: @thesahindia $250, please request on Newdot

@sakluger sakluger closed this as completed May 2, 2024
@JmillsExpensify
Copy link

$250 approved for @thesahindia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants