[PAID] [LOW] [$500] Mweb/Chrome - Status - In status, emoji selected is not shown in preview as selected

lanitochka17 commented 10 months 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!

Version Number: 1.4.18-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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Go to https://staging.new.expensify.com/
Tap profile icon --- Profile
Tap status
Tap emoji picker
Select an emoji

Expected Result:

In status, emoji selected must be shown in preview as selected

Actual Result:

In status, emoji selected is not shown in preview as selected

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[x] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/3e7cc161-c073-40a2-b4f2-07e037c81c33

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~018c9ea15b7ecaf817
Upwork Job ID: 1740170921969528832
Last Price Increase: 2024-01-18
Automatic offers:

getusha | Reviewer | 28121816
wildan-m | Contributor | 28121817

Issue Owner

Current Issue Owner: @strepanier03

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months 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

melvin-bot[bot] commented 10 months ago

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

shubham1206agra commented 10 months ago

@lanitochka17 Unable to repro this. Is there any special steps which I am missing?

bernhardoj commented 10 months ago

I think @lanitochka17 expects the status emoji is highlighted/focused on the emoji picker which sounds like a new feature

strepanier03 commented 10 months ago

I think @lanitochka17 expects the status emoji is highlighted/focused on the emoji picker which sounds like a new feature

I agree with you @bernhardoj, that does appear to be the expectation. I'll confirm internally if this is a good fit for one of our waves and update this once I have more info.

Note: We have a lot of folks OoO for the holidays so I don't expect an update until next week.

melvin-bot[bot] commented 10 months ago

@strepanier03, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 10 months ago

@strepanier03, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 10 months ago

@strepanier03, @getusha Huh... This is 4 days overdue. Who can take care of this?

strepanier03 commented 10 months ago

I bumped internally because I think we can either include this in a VIP as a new feature or we can close this entirely.

I tested in Slack and the behavior in Slack is the same as the behavior here, where the emoji isn't highlighted to indicate its selected, so at this time I'm leaning towards closing but will update when I know for sure.

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

strepanier03 commented 10 months ago

This is a LOW priority issue for the VIP-VSB.

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 10 months ago

@strepanier03 @getusha this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

strepanier03 commented 10 months ago

Low priority for now unless someone posts a proposal for review.

wildan-m commented 10 months ago

Proposal

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

Status - In status, emoji selected is not shown in preview as selected

What is the root cause of that problem?

No such feature yet

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

We can pass activeEmoji to EmojiPicker through showEmojiPickerand compare with each emoji.

If required, we can ignore skin tone by comparing its base emoji using this function

const getRemovedSkinToneEmoji = (emoji: string) => {
    return emoji.replace(CONST.REGEX.EMOJI_SKIN_TONES, '');
};

Where EMOJI_SKIN_TONES is /[\u{1f3fb}-\u{1f3ff}]/gu

To highlight the emoji we can modify this logic

https://github.com/Expensify/App/blob/176fb0011905c477c4e8ef3bb9ef472df4ae46d9/src/components/EmojiPicker/EmojiPickerMenu/index.js#L347

to

            const shouldEmojiBeHighlighted =
                (index === highlightedIndex && highlightFirstEmoji) || (activeEmoji && EmojiUtils.getRemovedSkinToneEmoji(emojiCode) === EmojiUtils.getRemovedSkinToneEmoji(activeEmoji));

If we want to consider skin tone (exact match) we can use emojiCode === activeEmoji instead

Branch to test.

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 10 months ago

@strepanier03, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!

getusha commented 10 months ago

Will review proposals.

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 10 months ago

@strepanier03 @getusha this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

strepanier03 commented 10 months ago

Thanks @getusha!

strepanier03 commented 9 months ago

@getusha - Any update on the proposals you reviewed?

strepanier03 commented 9 months ago

Not sure what's up with the Overdue label today 🤷‍♀️

strepanier03 commented 9 months ago

Starred this on myself hoping that works.

getusha commented 9 months ago

Testing @wildan-m's proposal will try to provide an update sooner.

wildan-m commented 9 months ago

Thanks @getusha

getusha commented 9 months ago

@wildan-m's proposal appears to work to highlight the emoji in the Emojipicker, i am not sure if we want to do that. i'll open a discussion in expensify-open-souce

getusha commented 9 months ago

Started a discussion https://expensify.slack.com/archives/C01GTK53T8Q/p1706117484845689

wildan-m commented 9 months ago

@getusha, @strepanier03 It appears to be a 'nice-to-have' feature. Notably, it can provide a slight efficiency boost for users; they can instantly know the currently active emoji without having to look back at the status button. would it be advisable for us to proceed and implement it?

getusha commented 9 months ago

Let's proceed with @wildan-m's proposal, and we can address any other cases as they arise during the PR review. 🎀 👀 🎀 C+ reviewed!

melvin-bot[bot] commented 9 months ago

Triggered auto assignment to @hayata-suenaga, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

wildan-m commented 9 months ago

@getusha, @hayata-suenaga in case we want to proceed, I have prepared a PR ready for review.

getusha commented 9 months ago

@wildan-m you should not raise a PR before assignment.

wildan-m commented 9 months ago

@getusha no worries we can just close it if it's not required, I have limited availability in the next hours, so it's just optimistic scenario

melvin-bot[bot] commented 9 months ago

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

Offer link Upwork job

melvin-bot[bot] commented 9 months ago

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

wildan-m commented 9 months ago

@hayata-suenaga, @getusha awesome! thanks for assigning me. PR Link

jjcoffee commented 9 months ago

Ideally I think we should hold this on https://github.com/Expensify/App/pull/34581 (will hopefully be merged soon).

hayata-suenaga commented 9 months ago

@jjcoffee let me know when the PR you linked is merged so that I can remove the hold

jjcoffee commented 9 months ago

@hayata-suenaga Merged now!

wildan-m commented 9 months ago

@jjcoffee thanks for letting us know.

@getusha The PR conflict has been resolved. PR Link

hayata-suenaga commented 9 months ago

yaaay! @getusha could you review the PR when you have time?

getusha commented 9 months ago

@hayata-suenag will review it.

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

https://github.com/Expensify/App/pull/35140

If no regressions arise, payment will be issued on 2024-02-15. :confetti_ball:

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

@wildan-m requires payment automatic offer (Contributor)
@getusha requires payment automatic offer (Reviewer)

melvin-bot[bot] commented 9 months ago

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:

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

wildan-m commented 9 months ago

@strepanier03 @getusha @hayata-suenaga seems no regression here(?)

hayata-suenaga commented 9 months ago

Yes I think the regression wait period is over @wildan-m please wait for others to complete the checklist 🙇

getusha commented 9 months ago

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:

[@getusha] The PR that introduced the bug has been identified. Link to the PR: new feature [@getusha] 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: new feature [@getusha] 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 it is a new feature [@getusha] Determine if we should create a regression test for this bug. Yes [@getusha] 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.

Regression Test Proposal

Go to account settings > Profile > Status
Pick a new emoji
Open the emoji picker
Verify that the picked emoji is highlighted inside the Emojipicker.

Do we agree 👍 or 👎

Expensify / App