[HOLD for regression test] [$1000] Tooltip not shown for emoji picker button on hover the outer part, it only shown for hovering the icon

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

Hover on the emojipicker button icon surrounding pressable area

Expected Result:

Tooltip should shown for entire button

Actual Result:

Tooltip only shows on hover the icon

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.37-2
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
ff5be293-b6dd-4ede-8cde-239f39379a3f.webm

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

View all open jobs on GitHub

Recording.1074.mp4

ff5be293-b6dd-4ede-8cde-239f39379a3f.webm

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e647d4e751c0a2f6
• Upwork Job ID: 1601033782775242752
• Last Price Increase: 2022-12-12

[melvin-bot]

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

[syedsaroshfarrukhdot]

Proposal :-

We need to wrap Pressable in Tooltip instead of the icon

diff --git a/src/components/EmojiPicker/EmojiPickerButton.js b/src/components/EmojiPicker/EmojiPickerButton.js
index 32af3ed59..f199850be 100644
--- a/src/components/EmojiPicker/EmojiPickerButton.js
+++ b/src/components/EmojiPicker/EmojiPickerButton.js
@@ -28,6 +28,7 @@ const defaultProps = {
 const EmojiPickerButton = (props) => {
     let emojiPopoverAnchor = null;
     return (
+        <Tooltip text={props.translate('reportActionCompose.emoji')}>
         <Pressable
             ref={el => emojiPopoverAnchor = el}
             style={({hovered, pressed}) => ([
@@ -39,14 +40,15 @@ const EmojiPickerButton = (props) => {
             nativeID={props.nativeID}
         >
             {({hovered, pressed}) => (
-                <Tooltip text={props.translate('reportActionCompose.emoji')}>
+               
                     <Icon
                         src={Expensicons.Emoji}
                         fill={StyleUtils.getIconFillColor(getButtonState(hovered, pressed))}
                     />
-                </Tooltip>
+               
             )}
         </Pressable>
+        </Tooltip>
     );
 };

After Fix :-

Screen.Recording.2022-12-09.at.5.36.35.AM.mov

[kadiealexander]

From this SO: https://stackoverflow.com/c/expensify/questions/14418

• The "bug" is actually a bug: Yes
• The bug is not a duplicate report of an existing GH (close the GH and add any novel details to the original GH instead): No
• The bug is reproducible, following the reproduction steps: Yes, I was able to reproduce this in the Desktop app.
• If you’re unable to reproduce the bug, add the Needs reproduction label. Comment on the issue outlining the steps you took to try to reproduce the bug, your results and tag the issue reporter and the Applause QA member who created the issue. Ask them to clarify reproduction steps and/or to check the reproduction steps again. Close issue.: n/a
• The GH template is filled out as fully as possible -- this means the GH body and title are clear (ie. could an external contributor understand it and work on it?): Yes
• The GH was created by an Expensify employee or a QA tester: Yes
• If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it’s better to wait, close the GH & provide this justification: n/a
• If there's a link to Slack, check the discussion to see if we decided not to fix it: n/a
• Decide if the GH should be resolved by an External contributor or Internal engineer, add the appropriate label: External should be fine here.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[Pujan92]

Proposal

Wrap Pressable inside the Tooltip, looks the same as the earlier other proposal but posting it after the external label is applied.

App/src/components/EmojiPicker/EmojiPickerButton.js

Lines 31 to 49 in 216bbe9

	<Pressable
	ref={el => emojiPopoverAnchor = el}
	style={({hovered, pressed}) => ([
	styles.chatItemEmojiButton,
	StyleUtils.getButtonBackgroundColorStyle(getButtonState(hovered, pressed)),
	])}
	disabled={props.isDisabled}
	onPress={() => EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor)}
	nativeID={props.nativeID}
	>
	{({hovered, pressed}) => (
	<Tooltip text={props.translate('reportActionCompose.emoji')}>
	<Icon
	src={Expensicons.Emoji}
	fill={StyleUtils.getIconFillColor(getButtonState(hovered, pressed))}
	/>
	</Tooltip>
	)}
	</Pressable>

     <Tooltip text={props.translate('reportActionCompose.emoji')}>
            <Pressable
                ref={el => emojiPopoverAnchor = el}
                style={({hovered, pressed}) => ([
                    styles.chatItemEmojiButton,
                    StyleUtils.getButtonBackgroundColorStyle(getButtonState(hovered, pressed)),
                ])}
                disabled={props.isDisabled}
                onPress={() => EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor)}
                nativeID={props.nativeID}
            >
                {({hovered, pressed}) => (
                    <Icon
                        src={Expensicons.Emoji}
                        fill={StyleUtils.getIconFillColor(getButtonState(hovered, pressed))}
                    />
                )}
            </Pressable>
        </Tooltip>

[kryptify]

Proposal:
You need to wrap whole pressable component with Tooltip. At this time, Tooltip is wrapping a Icon component.
That's why the tooltip is showing when mouse hover the Icon, not whole pressable.
Code is same as above.
My Upwork profile is :
https://www.upwork.com/freelancers/~01a1a69d7da842b796

[MunibQazi12]

Proposal
It is a simple problem to fix we need to just wrap the tooltip component into the pressable component.
This is a simple solution to this problem.
Here is my Upwork profile: https://www.upwork.com/freelancers/~01cf836885e7c7a016

[thienlnam]

@asimimamabdullah Please direct all setup and environment problems not related to the issue to #expensify-open-source here

As for the other conversation happening here, we're aware of the lack of clarity around proposals before the External label, and have a couple fixes that will be coming out soon. But yes, contributors should not be posting until the external label is applied. I'm going to minimize all conversation that is not related to the issue. If you have additional questions, please bring it to #expensify-open-source

[aimane-chnaif]

After internal discussion, proposals before External label is applied are still acceptable as stated in the current version of contributing.md.
We'll update contributing.md soon and announce this change on slack channel, so please keep 👁️ on this.

[Pujan92]

1 minor issue I am seeing for the above proposals is due to the vertical margin applied to the emoji button showing tooltip even though we hover on the top and bottom part.

Issue
4734cc5f-15ea-45aa-af1f-d70ed50a66f8.webm

Updated Proposal

Wrap Tooltip with View and remove vertical margin from this emoji button.

<View style={[styles.justifyContentCenter]}>
    <Tooltip text={props.translate('reportActionCompose.emoji')}>

App/src/styles/styles.js

Line 1482 in ed3e2dc

marginVertical: 4,

remove marginVertical property

9c0cb7b4-9cbf-4adc-b389-ef96cd1ebf05.webm

[syedsaroshfarrukhdot]

1 minor issue I am seeing for the above proposals is due to the vertical margin applied to the emoji button showing tooltip even though we hover on the top and bottom part.

I believe this is intentional as all Icons have margin and tool tips are visible while hovering at top and bottom of all buttons in chat composer. My Proposal #13455 (comment) was to match all other icons tooltip visibility in Chat Composer.

After Fix #13455 (comment) :-

Screen.Recording.2022-12-10.at.7.18.06.AM.mov

[Pujan92]

I don't think it is intentional and according to me, it might not be a good user experience. I have raised the same here. To make the item center I think it's better to use justifyContent instead of a fixed vertical margin.

[kadiealexander]

@aimane-chnaif any thoughts on the above?

[kadiealexander]

I'm going OOO for a family emergency, if this needs action while I'm away please re-add the bug label to assign someone.

[melvin-bot]

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

• show tooltip for emoji picker button on hover the outer part #13612

If no regressions arise, payment will be issued on 2022-12-27. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

• [@aimane-chnaif / @thienlnam] The PR that introduced the bug has been identified. Link to the PR:
• [@aimane-chnaif / @thienlnam] 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:
• [@aimane-chnaif / @thienlnam] 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:
• [@kadiealexander] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[situchan]

@situchan please apply on Upwork or let me know your name in Upwork so we can ensure you're paid :)

@kadiealexander applied. thank you

[JmillsExpensify]

I extended the contract while Kadie is out. In the meantime, we're still waiting on the regression period to pass.

[melvin-bot]

@thienlnam, @kadiealexander, @aimane-chnaif, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[aimane-chnaif]

For BugZero Checklist, I don't think any PR caused regression. The issue existed from the beginning when tooltip added to emoji picker button here.

[aimane-chnaif]

I extended the contract while Kadie is out. In the meantime, we're still waiting on the regression period to pass.

@JmillsExpensify regression period passed already

[aimane-chnaif]

@JmillsExpensify or @kadiealexander kindly bump ^

[JmillsExpensify]

I'm not assigned here but I can help. Assigning myself.

[JmillsExpensify]

It looks like we have the following:

• Issue reporter: @Pujan92 $250
• Contributor: @situchan $1,500 (incl. 50% bonus)
• Contributor+: @aimane-chnaif $1,500 (incl. 50% bonus).

Upwork job is here. All contributors have been paid out.

[JmillsExpensify]

That means we only lack the BZ checklist. @thienlnam do you agree with @aimane-chnaif that we're good on your items? If so, let's get those checked off.

[thienlnam]

Yeah I agree, checked those items off

[kadiealexander]

Thank you for your help @JmillsExpensify, I was unfortunately OOO sick. I've proposed a regression test update in Slack here :)

[JmillsExpensify]

Awesome thank you!

[melvin-bot]

@thienlnam, @kadiealexander, @aimane-chnaif, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[thienlnam]

Closing this out, from the discussion here there is no regression test needed https://expensify.slack.com/archives/C01SKUP7QR0/p1673912860873499