[$1500] Tooltip text are not centered in reaction emojis

[kavimuru]

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:

1. Open any group chat
2. React to a message by few of the group members
3. Hover over reaction emoji to show the tooltip

Expected Result:

Text should be centered

Actual Result:

Text is not centered

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
• [x] MacOS / Desktop

Version Number: 1.2.85-1 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:

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c2cc458e2af1d2c0
• Upwork Job ID: 1638556601537552384
• Last Price Increase: 2023-04-06

[MelvinBot]

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

[MelvinBot]

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]

@michaelhaxhiu Eep! 4 days overdue now. Issues have feelings too...

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[michaelhaxhiu]

Not overdue melvie :)

[PrashantMangukiya]

Proposal

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

Whenever multiple user has reacted among the group member, at that time tooltip text are not centered in reaction emojis.

What is the root cause of that problem?

Within ReactionTooltipContent.js file we are using <Text component. Text align center style is missing on that. This is the root cause of the problem. https://github.com/Expensify/App/blob/366f0d064f50a614ca42aca3586beba6bfaba8c7/src/components/Reactions/ReactionTooltipContent.js#L51-L58

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

Within ReactionTooltipContent.js file we have to add styles.textAlignCenter as shown below, it will solve the problem and long name string shows in centere as show in the result screenshot.

 <Text style={[ 
     styles.mt1, 
     styles.textMicroBold, 
     styles.textReactionSenders, 
     styles.textAlignCenter,    // *** ADD THIS CODE ****
 ]} 
 > 
     {namesString} 
 </Text> 

What alternative solutions did you explore? (Optional)

None

Other Suggestion

Within ReactionTooltipContent.js file I can see styles.textAlignCenter also missing for reacted with Text as shown below. So similar center align problem occur if reacted emoji name will be too long (I know such long emoji name is not there in our emoji list, but better to apply center align style so suggesting this) https://github.com/Expensify/App/blob/366f0d064f50a614ca42aca3586beba6bfaba8c7/src/components/Reactions/ReactionTooltipContent.js#L60-L66

So I will suggest to add styles.textAlignCenter for reacted with text also as shown below:

<Text style={[
    styles.textMicro,
    styles.fontColorReactionLabel,
    styles.textAlignCenter,    // *** ADD THIS CODE ****
]}
>
    {`reacted with :${props.emojiName}:`}
</Text>

Result

[0xmiros]

@PrashantMangukiya thanks for the quick proposal. Can you please remove old one since it seems duplicate? Edit: I see you deleted old comment now. Thanks

[dhairyasenjaliya]

Proposal

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

• Tooltip text is not centered in reaction emojis when we have multiple reactions by other members

What is the root cause of that problem?

• The actual cause of the problem is that we have not added any center alignment inside ReactionTooltipContent.js for namestring that makes the component by default left aligned and also we have not managed to keep the same lineHeight for text reacted with same lineHeight as emoji display at the top (IMO the lineheight of reacted with should be matched with top header emoji which makes tooltip looks clean) currently the reacted with is more aligned toward bottom

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

Changes 1

• To make the center align all reacted member's lists we need to add styles.textAlignCenter

 <Text style={[ 
     styles.mt1, 
     styles.textMicroBold, 
     styles.textReactionSenders, 
     styles.textAlignCenter,    // Line to be added in order to align all names into the center 
 ]} 
 > 
     {namesString} 
 </Text>         

Result of Change 1

Changes 2

• We need one more change to add lineHeight to reacted with to match with the header display emoji

• Currently the lineheight for reacted with is too small making the component vertically stick which IMO looks a bit weird and makes conflict to read

• iconSizeXLarge lineHeight used by top emoji display same we can use for reacted with

• Of course, this changes we need to confirm these changes from the design team cc @shawnborton

    <Text style={[
        styles.textMicro,
        styles.fontColorReactionLabel,
  +        lineHeight: variables.iconSizeXLarge,  <- This lineHeight needs to be added from styles.js this is just a reference 
       ]}
     >
      {`reacted with :${props.emojiName}:`}
  </Text>   

  Result After changes 1 & 2

Result before change 2 (current state)

What alternative solutions did you explore? (Optional)

• None

[0xmiros]

@PrashantMangukiya's proposal will do the job. The solution is straightforward.

@dhairyasenjaliya thanks for your proposal, but Changes 2 is out of scope for this issue. If you think it should be fixed, please report in slack channel.

🎀 👀 🎀 C+ reviewed cc: @techievivek

[MelvinBot]

📣 @PrashantMangukiya You have been assigned to this job by @techievivek! 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 📖

[techievivek]

@PrashantMangukiya Thanks for the proposal, you should be good to push a PR now.

[PrashantMangukiya]

Thanks @techievivek Applied for the job, Submitting PR within hour asap.

[PrashantMangukiya]

@techievivek PR for this issue deployed to production, but issue title not changed and Reviewing label not removed. Some automation glitch. I think title should be changed with[HOLD for payment 2023-04-05] and Reviewing label needs to remove manually. Thanks.

[PrashantMangukiya]

Ping for Upwork.

[techievivek]

@michaelhaxhiu There seems to be some automation glitch here can you please have a look? Thanks

[PrashantMangukiya]

@michaelhaxhiu Ping for Upwork.

[michaelhaxhiu]

looking 👀

[michaelhaxhiu]

Summary:

March 27 - job assigned March 28 - PR merged

So 50% bonus for C and C+ 😌

[MelvinBot]

Upwork job price has been updated to $1500

[PrashantMangukiya]

@michaelhaxhiu Offer accepted on upwork.

[michaelhaxhiu]

This automation is ded 😫

[michaelhaxhiu]

Paid @PrashantMangukiya 👍

@0xmiroslav can you accept the invite in upwork and ping me here when it's done?

[PrashantMangukiya]

@michaelhaxhiu Received. Thank you for your time.

[michaelhaxhiu]

I think we may have some checklist work still btw!

[michaelhaxhiu]

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:

• [x] [@0xmiroslav / @techievivek] The PR that introduced the bug has been identified. Link to the PR:
• [x] [@0xmiroslav / @techievivek ] 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:
• [x] [@0xmiroslav / @techievivek ] 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:
• [x] [@michaelhaxhiu ] Determine if we should create a regression test for this bug.
• [x] [@0xmiroslav] 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.
• [x] [@michaelhaxhiu ] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[techievivek]

@0xmiroslav Do you think we need to add a regression test to ensure this bug doesn't reappear?

[0xmiros]

I don't think anyone will introduce regression (center aligned -> left aligned) unless intentional or tooltip is completely refactored

[0xmiros]

But it would be good if we can add regression test step something like "whenever new tooltip is added, verify that text is always centered"

[techievivek]

I think we can skip the regression test for this one. Thanks for the input.

[techievivek]

@michaelhaxhiu If everyone is paid, we can close this out.

[MelvinBot]

@PrashantMangukiya @michaelhaxhiu @techievivek @0xmiroslav this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[MelvinBot]

Current assignee @0xmiroslav is eligible for the Internal assigner, not assigning anyone new.

[0xmiros]

@0xmiroslav can you accept the invite in upwork and ping me here when it's done?

@michaelhaxhiu I accepted invite already.

[michaelhaxhiu]

all parties paid and checkboxes ✅ 'ed. Closing.