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

[Hold for payment 01-02-2023] [$1000] Tooltip is not in the center while hovering on a profile pic #13940

Closed
1 task
kavimuru opened this issue Jan 2, 2023 · 48 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Jan 2, 2023

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:

  1. Open any chat
  2. hover mouse on profile pic

Expected Result:

tooltip should be in center of profile pic

Actual Result:

tooltip is not in the center

Workaround:

unknown

Platforms:

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

  • Web / Chrome / Safari

Version Number: 1.2.47-0
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:
Screenshot 2022-12-28 at 3 22 25 PM
Untitled

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e3f915ebae1ec606
  • Upwork Job ID: 1610419163878563840
  • Last Price Increase: 2023-01-03
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 2, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 2, 2023
@trjExpensify
Copy link
Contributor

I can repro this reliably, moving external.

@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label Jan 3, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 3, 2023
@melvin-bot melvin-bot bot changed the title Tooltip is not in the center while hovering on a profile pic [$1000] Tooltip is not in the center while hovering on a profile pic Jan 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jan 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jan 3, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 3, 2023

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

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 3, 2023

Proposal

Issue is occuring due to the margin applies to the child component of the Tooltip.
To fix we need to apply margin right to the Pressable(parent component of tooltip).

diff --git a/src/pages/home/report/ReportActionItemSingle.js b/src/pages/home/report/ReportActionItemSingle.js
index a4958ab1b..4ad15b159 100644
--- a/src/pages/home/report/ReportActionItemSingle.js
+++ b/src/pages/home/report/ReportActionItemSingle.js
@@ -66,7 +66,7 @@ const ReportActionItemSingle = (props) => {
     return (
         <View style={props.wrapperStyles}>
             <Pressable
-                style={styles.alignSelfStart}
+                style={[styles.alignSelfStart, styles.mr2]}
                 onPressIn={ControlSelection.block}
                 onPressOut={ControlSelection.unblock}
                 onPress={() => showUserDetails(props.action.actorEmail)}
diff --git a/src/styles/styles.js b/src/styles/styles.js
index b3de12803..f4d161c59 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -695,7 +695,6 @@ const styles = {
     // Actions
     actionAvatar: {
         borderRadius: 20,
-        marginRight: 8,
     },

We can replace borderRadius: 20 with the variable too.
borderRadius: variables.componentBorderRadiusRounded

Screenshot 2023-01-03 at 6 50 07 PM

@bondydaa
Copy link
Contributor

bondydaa commented Jan 3, 2023

This seems okay though I think it does end up making the profile pictures ever so slightly smaller as well

image

cc @shawnborton thoughts?

@bondydaa
Copy link
Contributor

bondydaa commented Jan 4, 2023

oh wait no nevermind it's not smaller, zooming in on that and using something for guide they're the same size
image

👍

@bondydaa
Copy link
Contributor

bondydaa commented Jan 4, 2023

Will wait for the C+ to confirm as well

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 4, 2023

@bondydaa Currently that avatar contains the right margin which increases the width of the Tooltip area and due to that center of the area is a bit right and not within the center of the avatar.

Screenshot 2023-01-04 at 9 45 09 AM

@shawnborton
Copy link
Contributor

I'm not sure at what point this changed but there should be 12px margin (not 8px) in between the avatar and the name/message. We could fix that here or we could save that for a new issue.

@shawnborton
Copy link
Contributor

Actually sorry, let's tackle the extra margin as a separate issue - I think there are other implications here we need to consider beyond the tooltip so I don't want to bloat the original issue at hand. Sorry about that!

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 4, 2023

Yes, 12px for particular this case will shift the content more to the right which will be inconsistent with the other messages alignment and requires a fix at other places too.

@Puneet-here
Copy link
Contributor

Proposal

This issue is present for name tooltip too.
Screenshot 2023-01-04 at 4 03 56 PM

to fix it we need to remove the padding from below

App/src/styles/styles.js

Lines 1315 to 1316 in 034a6f0

paddingRight: 5,
paddingBottom: 4,

Add the padding styles below

style={[styles.flexShrink1]}

To fix the avatar tooltip issue we need to add styles.mr2 below

style={styles.alignSelfStart}

and remove margin here
marginRight: 8,

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 4, 2023

@shawnborton just to correct the current margin in the LHN for avatar and name is 16px.

Screenshot 2023-01-04 at 4 20 34 PM

To make the same 16px margin for the messages we can apply below changes.

Screenshot 2023-01-04 at 4 25 10 PM

diff --git a/src/pages/home/report/ReportActionItemSingle.js b/src/pages/home/report/ReportActionItemSingle.js
index a4958ab1b..fdb1464be 100644
--- a/src/pages/home/report/ReportActionItemSingle.js
+++ b/src/pages/home/report/ReportActionItemSingle.js
@@ -66,7 +66,7 @@ const ReportActionItemSingle = (props) => {
     return (
         <View style={props.wrapperStyles}>
             <Pressable
-                style={styles.alignSelfStart}
+                style={[styles.alignSelfStart, styles.mr4]}
                 onPressIn={ControlSelection.block}
                 onPressOut={ControlSelection.unblock}
                 onPress={() => showUserDetails(props.action.actorEmail)}
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 82e9ca8f2..4005ff276 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -695,7 +695,6 @@ const styles = {
     // Actions
     actionAvatar: {
         borderRadius: 20,
-        marginRight: 8,
     },
 
     componentHeightLarge: {
@@ -1289,7 +1288,7 @@ const styles = {
         flexShrink: 1,
         flexBasis: 0,
         position: 'relative',
-        marginLeft: 48,
+        marginLeft: 56,
     },

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 4, 2023

@shawnborton just to correct the current margin in the LHN for avatar and name is 16px.

But to make it consistent we need to apply changes to the display user time line and in the composer input too, which seems to be the reason to me why it is 8px.

Screenshot 2023-01-04 at 4 53 49 PM

@umair-upwork
Copy link

umair-upwork commented Jan 4, 2023

Proposal

Problem

This issue is occurring because of the margin added to the <Avatar> with styles.actionAvatar, which has a marginRight of 8. The <Avatar>is inside of the <Tooltip>, so adding a marginRight to <Avatar> also adds some margin to <Tooltip>, causing it to shift to the right instead of remaining centered.

<Avatar
containerStyles={[styles.actionAvatar]}

Solution

We can remove styles.actionAvatar from <Avatar> in

<Avatar
containerStyles={[styles.actionAvatar]}

and add it to <Pressable style={[styles.alignSelfStart, styles.actionAvatar]}> in

<Pressable
style={styles.alignSelfStart}

Now, if we add any margin value to it in

App/src/styles/styles.js

Lines 696 to 699 in 034a6f0

actionAvatar: {
borderRadius: 20,
marginRight: 8,
},

Tooltip will always remain centered while hovering on a profile pic or name

Demo

Screen.Recording.2023-01-04.at.4.32.00.PM.mov

@syedsaroshfarrukhdot
Copy link
Contributor

syedsaroshfarrukhdot commented Jan 4, 2023

Proposal :-

The same issue is being replicated for name as highlighted by @Puneet-here

As there are multiple ways to solve it I find this solution to be best working if we in future we need to change paddings for either item for name and avatar Tooltip position won't be affected it will be always be centered for both name and avatar.

We can pass avatar style to presssable and marginLeft : 5 To ReportActionItemDate

diff --git a/src/pages/home/report/ReportActionItemDate.js b/src/pages/home/report/ReportActionItemDate.js
index 0cf570132..02e928cf9 100644
--- a/src/pages/home/report/ReportActionItemDate.js
+++ b/src/pages/home/report/ReportActionItemDate.js
@@ -13,7 +13,7 @@ const propTypes = {
 };
 
 const ReportActionItemDate = props => (
-    <Text style={[styles.chatItemMessageHeaderTimestamp]}>
+    <Text style={[styles.chatItemMessageHeaderTimestamp, {...props.style}]}>
         {props.datetimeToCalendarTime(props.created)}
     </Text>
 );
diff --git a/src/pages/home/report/ReportActionItemSingle.js b/src/pages/home/report/ReportActionItemSingle.js
index a4958ab1b..980208df2 100644
--- a/src/pages/home/report/ReportActionItemSingle.js
+++ b/src/pages/home/report/ReportActionItemSingle.js
@@ -66,14 +66,13 @@ const ReportActionItemSingle = (props) => {
     return (
         <View style={props.wrapperStyles}>
             <Pressable
-                style={styles.alignSelfStart}
+                style={[styles.alignSelfStart, styles.actionAvatar]}
                 onPressIn={ControlSelection.block}
                 onPressOut={ControlSelection.unblock}
                 onPress={() => showUserDetails(props.action.actorEmail)}
             >
                 <Tooltip text={props.action.actorEmail}>
                     <Avatar
-                        containerStyles={[styles.actionAvatar]}
                         source={avatarUrl}
                     />
                 </Tooltip>
@@ -98,7 +97,7 @@ const ReportActionItemSingle = (props) => {
                                 />
                             ))}
                         </Pressable>
-                        <ReportActionItemDate created={props.action.created} />
+                        <ReportActionItemDate created={props.action.created} style={{marginLeft : 5}}/>
                     </View>
                 ) : null}
                 {props.children}
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 82e9ca8f2..be7715143 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -1312,7 +1312,6 @@ const styles = {
         fontSize: variables.fontSizeNormal,
         fontWeight: fontWeightBold,
         lineHeight: variables.lineHeightXLarge,
-        paddingRight: 5,
         paddingBottom: 4,
         ...wordBreak.breakWord,
     },

After Fix :-

Screen.Recording.2023-01-04.at.6.15.07.PM.mov

@daraksha-dk
Copy link
Contributor

coming from here

Proposal

I found this tooltip not being centered issue throughout the app and compiled a list of changes to fix all of them.

There are two main sections where this issue resides:

1. for Header buttons

  • Places where HeaderWithCloseButton is being used
  • IOUModal

Changes that needs to be done:

Adding a View Container with style prop having styles.mr2 as value and setting styles.mr0 for the Pressable Component below:

{this.props.shouldShowBackButton && (
<Tooltip text={this.props.translate('common.back')}>
<Pressable
onPress={() => {
if (this.props.isKeyboardShown) {
Keyboard.dismiss();
}
this.props.onBackButtonPress();
}}
style={[styles.touchableButtonImage]}
>
<Icon src={Expensicons.BackArrow} />
</Pressable>
</Tooltip>
)}

image

Doing the same for download button here

{
this.props.shouldShowDownloadButton && (
<Tooltip text={this.props.translate('common.download')}>
<Pressable
onPress={this.triggerButtonCompleteAndDownload}
style={[styles.touchableButtonImage]}
>
<Icon
src={Expensicons.Download}
fill={StyleUtils.getIconFillColor(getButtonState(false, false, this.props.isDelayButtonStateComplete))}
/>
</Pressable>
</Tooltip>
)
}

Although I think the download button doesn't need extra margin of 8px in the right as in all the other places the buttons are right next to each other without extra padding/margin. Will confirm this with @shawnborton at the bottom.

Same changes for the IOUModal here

{this.state.currentStepIndex > 0
&& (
<Tooltip text={this.props.translate('common.back')}>
<TouchableOpacity
onPress={this.navigateToPreviousStep}
style={[styles.touchableButtonImage]}
>
<Icon src={Expensicons.BackArrow} />
</TouchableOpacity>
</Tooltip>
)}

Another way to solve these problems (IMO Better):

We can just remove line no. 525 from here and make the rest of the changes accordingly because in all the other places where this style is being used we're eventually resetting the margin-right value by using styles.mr0

App/src/styles/styles.js

Lines 521 to 527 in 034a6f0

touchableButtonImage: {
alignItems: 'center',
height: variables.componentSizeNormal,
justifyContent: 'center',
marginRight: 8,
width: variables.componentSizeNormal,
},

2. for Avatar and Name tooltips

Changes

Remove these lines of code:

App/src/styles/styles.js

Lines 1315 to 1316 in 034a6f0

paddingRight: 5,
paddingBottom: 4,

marginRight: 8,

paddingRight:5,
paddingBottom: 4,

Create a new object with above styles and pass below:

style={[styles.flexShrink1]}

And add styles.mr2 to

style={styles.alignSelfStart}


Places where the buttons are stacked right next to each other without any spacing :
image
image

The only place where a gap of 8px is observed
image

Just want to confirm if this is intentional or need to be fixed.
cc: @shawnborton

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 4, 2023

Just to make a point, the items mentioned by the other contributors are suggestions and improvements which is not directly related to the raised issue. I definitely agree that these are the bugs that can be raised in the future and solving them now is a good step but I believe the first valid proposal for the raised issue needs to be considered and should get a chance to incorporate these changes(all tooltip positional) if we are planning for it.

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 6, 2023

PR is ready for review.

@Puneet-here
Copy link
Contributor

Hi, I wanted to confirm if I am eligible for compensation since I mentioned a new instance of this issue and proposed a solution which was used. Like here.

@melvin-bot
Copy link

melvin-bot bot commented Jan 13, 2023

@bondydaa, @eVoloshchak, @Pujan92, @trjExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@eVoloshchak
Copy link
Contributor

eVoloshchak commented Jan 13, 2023

Not overdue, PR was deployed to production 3 days ago
Weird, there is no Overdue label

@jatinsonijs
Copy link
Contributor

@Pujan92 this issue still exist

Screenshot 2023-01-19 at 5 57 11 PM

@Pujan92
Copy link
Contributor

Pujan92 commented Jan 19, 2023

Ohh, seems this PR from @grgia overwritten changes which I did for this fix. The right margin which we applied to the parent element is applied again within the child in that PR which causes the issue. @bondydaa @eVoloshchak needed the below change to apply 12px margin between avatar and name.

diff --git a/src/pages/home/report/ReportActionItemSingle.js b/src/pages/home/report/ReportActionItemSingle.js
index bb1d39e62..c90335a9d 100644
--- a/src/pages/home/report/ReportActionItemSingle.js
+++ b/src/pages/home/report/ReportActionItemSingle.js
@@ -73,7 +73,7 @@ const ReportActionItemSingle = (props) => {
     return (
         <View style={props.wrapperStyles}>
             <Pressable
-                style={[styles.alignSelfStart]}
+                style={[styles.alignSelfStart, styles.mr3]}
                 onPressIn={ControlSelection.block}
                 onPressOut={ControlSelection.unblock}
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 71fed0201..23697f51d 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -709,7 +709,6 @@ const styles = {
     // Actions
     actionAvatar: {
         borderRadius: 20,
-        marginRight: variables.avatarChatSpacing,
     },

@bondydaa
Copy link
Contributor

oh hm dang I do not know what we're supposed to do here, going to ask in slack.

@bondydaa
Copy link
Contributor

to recap the slack discussion we're just going to have @Pujan92 re-add the fixes into the PR #14247 to get it re-applied.

Georgia has been OOO and since normally we'd get the PR author who "broke" it to fix it let's bypass that this one time to get the fixes back in place and keep moving forward on these tooltip problems.

@bondydaa
Copy link
Contributor

slack thread was here for posterity https://expensify.slack.com/archives/C01GTK53T8Q/p1674149953675229

@jatinsonijs
Copy link
Contributor

@bondydaa just want to confirm do it will consider as regression report ?

#13940 (comment)

@bondydaa
Copy link
Contributor

yeah I think so 👍

@bondydaa
Copy link
Contributor

merged #14247 which is taking care of fixing the regression that got added.

@trjExpensify trjExpensify changed the title [$1000] Tooltip is not in the center while hovering on a profile pic [Hold for payment 01-02-2023] [$1000] Tooltip is not in the center while hovering on a profile pic Feb 1, 2023
@trjExpensify
Copy link
Contributor

This hit prod 7 days ago (Wednesday, 1st Feb). Title didn't update, but doing that now to track it better. Is this the correct summary of payments due?

$250 for @gadhiyamanan for the bug report
$250 for @jatinsonijs for the bug report after someone else's PR broke this again
$1,500 for @Pujan92 for the fix + 50% bonus for PR merged within 3 days
$1,500 to @eVoloshchak for the C+ review + 50% bonus for PR merged within 3 days (we aren't counting someone's regression against this PR)

Let me know and I can ship the offers. Thanks!

@Puneet-here
Copy link
Contributor

Hi @trjExpensify, I wanted to confirm if I am eligible for compensation since I mentioned a new instance of this issue and proposed a solution which was used. A similar case was here.
Screenshot_2023_0201_111434

@Pujan92
Copy link
Contributor

Pujan92 commented Feb 1, 2023

@Puneet-here Just sharing my similar case here where it has counted as a suggestion. 😅

@trjExpensify
Copy link
Contributor

Hey @Puneet-here, can you be more specific/direct with what your implying, please? I don't know what you're pointing to there with @JmillsExpensify's summary of payments on another issue? 😕

The way I see it, this issue was created from a bug reported by @gadhiyamanan in #expensify-bugs as per the OP. We don't compensate for suggestions, and the solution we accepted was @Pujan92's.

@Puneet-here
Copy link
Contributor

Puneet-here commented Feb 1, 2023

Sorry for not being specific earlier. I was sharing a similar case where two contributors were compensated because the code that was use in the final PR was from both contributors (me and allrounderexperts)

daraksha-dk mentioned one new instance of this issue and provided a solution, in their case a new issue was created and the issue was assigned to them for solving.

I had also added a new instance of this issue and provided the solution to fix that. The solution was used in the final PR. That's why I wanted to confirm if I am eligible for compensation since it is similar to above cases.

@trjExpensify
Copy link
Contributor

Thanks! @daraksha-dk's issue was for the back/header buttons, which was treated separately because it had a different implementation as per the outcome of this thread.

Your proposal on this issue wasn't accepted unfortunately, and the initial report came from @gadhiyamanan, so I don't think we treat it as an additional bug report - like we are the one where @jatinsonijs found a regression from another PR that reverted these changes.

@trjExpensify
Copy link
Contributor

trjExpensify commented Feb 1, 2023

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:

@Puneet-here
Copy link
Contributor

By new "instance" I meant I reported and provided solution for a new place where this bug was present (it was also present in names, here's my comment ) that instance was not part of the original issue but we decided to fix it here and since pujan had proposed the solution first for the original issue their proposal was taken and we included the fix for the new instance in the same PR so I think my case is similar as darkasha-dk 's, but in my case a new issue wasn't created.

@trjExpensify
Copy link
Contributor

Ahh, I see. Thanks for elaborating on that, so yes, I'll ship you a bug reporting bonus.

@trjExpensify
Copy link
Contributor

All done here!

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 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