-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Cloudpresser/user details tooltip #20276
Cloudpresser/user details tooltip #20276
Conversation
@puneetlath @fedirjh One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Still waiting for a high-traffic account at luiz+high-traffic-1@cloudpresser.com |
I have read the CLA Document and I hereby sign the CLA |
1d3790d
to
a00dfab
Compare
@cloudpresser let's make sure the tooltip is also getting the proper styling. And can you try setting a handle manually in Onyx to show what it looks like with the handle as well. |
17b6680
to
788a2bd
Compare
Sounds good, I'll work on this after lunch today. Should have an update in 2-3hrs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good , left few comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An extra suggestion
@cloudpresser For style consistency , we can use same style used in ReactionTooltipContent.js App/src/components/Reactions/ReactionTooltipContent.js Lines 44 to 59 in 5739db4
|
This comment was marked as resolved.
This comment was marked as resolved.
In order for to handle the case with no handle or name, the dimensions of the overall tooltip should probably use paddings instead of a fixed height. Looking at the screenshots I can only guess the value for those margins, could you tell me:
Alternatively I could extract it from the design files, but I'm not sure where or if I can access those. |
Are you suggesting I use this for the name? It looks like these styles do not match the ones on the screenshot @puneetlath shared. I was looking at
|
I am suggesting to use same template , same style classes , change emoji code with avatar , emoji name with handle ... |
That makes sense, and saves me from guessing on those paddings. Thank you for the suggestion and for clarifying |
@cloudpresser I have got this bug , where the tooltip position in not stable , causing this layout glitches Bug.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloudpresser We should update all tooltips instances that display the user's email address
Examples :
- LHN
- Search Page
- Chat Header
- Details Page
- Threads
- Mentions
/> | ||
</View> | ||
|
||
{Boolean(props.name.trim()) ? <Text style={[styles.mt1, styles.textMicroBold, styles.textReactionSenders, styles.textAlignCenter]}>{props.name}</Text> : ''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{Boolean(props.name.trim()) ? <Text style={[styles.mt1, styles.textMicroBold, styles.textReactionSenders, styles.textAlignCenter]}>{props.name}</Text> : ''} | |
{Boolean(props.name.trim()) ? <Text style={[styles.mt1, styles.textStrong, styles.textAlignCenter]}>{props.name}</Text> : ''} |
We should use bold Style for name , I think we can create new style class for name that includes font-size and line-Height along side with styles.textStrong
@cloudpresser Just a quick reminder that this PR is highly important and should be merged this week. Could you please provide an update on when we can expect it to be ready? |
Sorry, one new request. Can we have the third line be login instead of handle. Looks like we're not going to get the back-end update to add handles in time. However, we should still gracefully handle the login field not being there. |
@puneetlath No problem, I'll get that in. @fedirjh, I'm expecting to finish it within the next 1-2 hours. Only variable I'm not sure how long it'll take is the bug @fedirjh reported. Hopefully is simple, I'm not sure it was caused by my changes, but maybe in combination with. Investigating... |
@fedirjh I have not been granted a high traffic account yet. My understanding is I should use that to test cases like the one you found a bug in, is that right? I requested it twice so far on slack |
@cloudpresser just made luiz+high-traffic-1@cloudpresser.com a high traffic account. |
Still working on this. The tooltips I had not accounted for are a bit of a deeper hole than anticipated. Will need to come back to it after lunch time. Expect another update before 5pm EST. |
Reviewer Checklist
Screenshots/Videos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work @cloudpresser , It looks good and tests well for all cases 🚀 🚀
All yours @puneetlath
Thank you @fedirjh @puneetlath for all the help and reviews! I learned a lot about the codebase with this PR |
Thank you both so much for your persistence and urgency on this one! It's really much appreciated. Looking forward to more contributions from you @cloudpresser. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.27-0 🚀
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.27-0 🚀
|
@@ -873,7 +884,8 @@ function getDisplayNameForParticipant(login, shouldUseShortForm = false) { | |||
function getDisplayNamesWithTooltips(participants, isMultipleParticipantReport) { | |||
return _.map(participants, (participant) => { | |||
const displayName = getDisplayNameForParticipant(participant.login, isMultipleParticipantReport); | |||
const tooltip = participant.login ? Str.removeSMSDomain(participant.login) : ''; | |||
const avatar = UserUtils.getDefaultAvatar(participant.login); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this caused a regression where we are unable to load a user's uploaded/custom avatar in some cases as identified on #20683
I believe I've got a fix so will push it up and request reviews from ya'll.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is here #20727
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bondydaa The root cause of that issue is that we are not passing the accountID
. The avatar you are referring to should only be displayed in case we have a user without an accountID
(I believe these cases are mentions or when you search for a new account).
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.27-7 🚀
|
I just reported this, but it seems we didn't handle the case for Workspace Avatars https://expensify.slack.com/archives/C049HHMV9SM/p1686821040533159 |
* @param {String} login | ||
* @returns {String} | ||
*/ | ||
function getAccountIDForLogin(login) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloudpresser @puneetlath
This method no longer works after accountID
migration because we have accountID
as they and not login
anymore. This is causing the following issues:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue reported here: #20934
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@esh-g If the the method no longer works after accountID
migration , then the migration PR caused this regression , it broke this function which was working fine before the migration.
So it's not a regression from this PR.
<Text style={[styles.textMicro, styles.fontColorReactionLabel]}> | ||
{String(userDetails.login).trim() && !_.isEqual(userDetails.login, userDetails.displayName) ? Str.removeSMSDomain(userDetails.login) : ''} | ||
</Text> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user with mobile number login case was handled incorrectly resulting in the regression showing the phone number twice with different formatting. Handled it by improving the checks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added the login handling here, but we didn't account for overflowing content on the tooltip with a very long email. This caused a regression here.
return props.children; | ||
} | ||
|
||
return <Tooltip renderTooltipContent={renderTooltipContent}>{props.children}</Tooltip>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tooltip
supports renderTooltipContentKey
prop for resize when renderTooltipContent
dynamically changes. As it was missed to pass this key, caused minor regression below:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tooltip
also supports shiftHorizontal
to adjust horizontal position of tooltip. As it was missed to pass this key, caused another minor regression below:
This PR adds UserTooltip and while testing we failed to catch inconsistency in the user tooltip due to a missing account id from the new task page. |
cc @Santhosh-Sellavel, I don't think this PR introduced that regression. It's probably a regression from : In this PR we used the name to get the App/src/components/MultipleAvatars.js Lines 80 to 81 in a0ea50d
|
@@ -74,7 +77,14 @@ const MultipleAvatars = (props) => { | |||
|
|||
if (props.icons.length === 1 && !props.shouldStackHorizontally) { | |||
return ( | |||
<Tooltip text={tooltipTexts[0]}> | |||
<UserDetailsTooltip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't properly migrate the shouldShowTooltip
prop to UserDetailsTooltip
which was being applied indirectly to the old Tooltip
via tooltipTexts at line 72.
Details
accountID, and fallbackUserDetails
Fixed Issues
$ #20086
$ #20086 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Does not apply (component does not render on mobile)Mobile Web - Safari
Does not apply (component does not render on mobile)Desktop
iOS
Does not apply (component does not render on mobile)Android
Does not apply (component does not render on mobile)