-
Notifications
You must be signed in to change notification settings - Fork 3k
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 2023-04-03] [$1000] App displays 'reacted with emoji' tooltip even on emojis in message right click menu #16050
Comments
Triggered auto assignment to @bfitzexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
Job added to Upwork: https://www.upwork.com/jobs/~010dc0bb4e0b87c1dc |
Current assignee @bfitzexpensify is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel ( |
Triggered auto assignment to @neil-marcellini ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Reaction in the context menu shows 2 tooltips What is the root cause of that problem?We have 2 tooltips on reaction emoji bubbles in the context menu so it shows 2 tooltip(first the emoji name & then the renderTooltipContent) App/src/components/Reactions/QuickEmojiReactions/BaseQuickEmojiReactions.js Lines 45 to 46 in 0efe6ac
App/src/components/Reactions/EmojiReactionBubble.js Lines 66 to 73 in 0efe6ac
What changes do you think we should make in order to solve the problem?Move the tooltip with renderTooltipContent from EmojiReactionBubble to App/src/components/Reactions/ReportActionItemReactions.js Lines 71 to 78 in 0efe6ac
With that Context menu reactions will have a single tooltip and solves the issue. Result Screen.Recording.2023-03-17.at.1.36.50.AM.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.Within emoji reaction popover (when right click any message) it shows 'reacted with emoji' within tooltip content. It should only display emoji name on hover of that 4 emojis with Add reaction button. App should display tooltip with reacted user info for all reacted emojis listed below the message) What is the root cause of that problem?We are passing App/src/components/Reactions/QuickEmojiReactions/BaseQuickEmojiReactions.js Lines 45 to 54 in 0efe6ac
So at present there is not any provision to decide that What changes do you think we should make in order to solve the problem?Within <Tooltip text={`:${emoji.name}:`} key={emoji.name} focusable={false}>
<EmojiReactionBubble
...
showReactedInfo={false} // *** ADD THIS ***
/>
</Tooltip> Within <Tooltip
showReactedInfo={props.showReactedInfo} // *** ADD THIS ***
renderTooltipContent={() => (
<ReactionTooltipContent
emojiName={props.emojiName}
emojiCodes={props.emojiCodes}
accountIDs={props.reactionUsers}
/>
)}
...
> Within {this.state.isRendered && ( // *** OLD CODE ***
{this.state.isRendered && this.props.showReactedInfo && ( // *** UPDATED CODE ***
<TooltipRenderedOnPageBody
...
/>
)} Add const propTypes = {
...
showReactedInfo: PropTypes.bool, // *** ADD THIS ***
};
const defaultProps = {
...
showReactedInfo: true, // *** ADD THIS ***
}; So this will solve the problem. Note: At present I decide prop name just to explain logic, we can change it as per suggestion. What alternative solutions did you explore? (Optional)None. Resulttooltip-result.mov |
Please re-state the problem that we are trying to solve in this issue. What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
for the above function, you need to pass the following param to
also, you need to add params to below mention file after line#79 to remove confliction to other mini icon on hover like copy to clipboard, delete etc. App/src/components/ContextMenuItem.js Line 79 in 0efe6ac
Video File: |
ProposalPlease re-state the problem that we are trying to solve in this issue.App displays 'reacted with emoji' tooltip even on emojis in message right click menu What is the root cause of that problem?Because the "right click" menu reuse the component App/src/components/Reactions/QuickEmojiReactions/BaseQuickEmojiReactions.js Lines 42 to 55 in 0b24114
Because component
That's reason, why we see "reacted with" text in the "right click" menu when hover in emoji. What changes do you think we should make in order to solve the problem?The "right click" menu should not use component Something like this: const QuickEmojiReaction = props => (
<Tooltip text={`:${props.emojiName}:`}>
<Pressable
style={} # TBD, but style might same as EmojiReactionBubble
onPress={props.onPress}
onLongPress={props.onReactionListOpen}
>
<Text style={} > # TBD, but style might same as EmojiReactionBubble
{props.preferredEmojiCode}
</Text>
</Pressable>
</Tooltip>
); What alternative solutions did you explore? (Optional)
|
@Pujan92 proposal LGTM, is simple and straightforward to me. Let me know your thoughts @neil-marcellini C+ Reviewed |
Waiting for CME review! |
📣 @Pujan92 You have been assigned to this job by @neil-marcellini! |
@Santhosh-Sellavel @neil-marcellini PR is ready for review. |
Looks like something related to As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our Feel free to drop a note in #expensify-open-source with any questions. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.89-0 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 2023-04-03. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
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:
|
Sent offers out on upwork |
|
@Pujan92, @neil-marcellini, @bfitzexpensify, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick! |
1 similar comment
@Pujan92, @neil-marcellini, @bfitzexpensify, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick! |
I think we should add a regression test for this to this WIP planned 'Emoji Reactions & Emoji suggestions' test - https://github.com/Expensify/Expensify/issues/274613 |
@bfitzexpensify Added steps 4 and 5 to cover the tooltip here https://github.com/Expensify/Expensify/issues/274613. |
Awesome thanks @isagoico! I think we are all done here then and can close this out. |
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:
Expected Result:
App should only display name of that emoji on hover of emojis in message right click menu
Actual Result:
App displays 'reacted with emoji' tooltip even on emojis in message right click menu (App should only display that tooltip on reacted emojis below the message)
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.86-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:
reacted.with.emoji.tooltip.issue.mp4
Recording.1718.mp4
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1678951003713649
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: