Only render UserDetailsTooltip on web#21807
Conversation
|
@sobitneupane Please 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] |
roryabraham
left a comment
There was a problem hiding this comment.
@hannojg few things:
- There are some conflicts to resolve here
- Instead of using
withLocalizeand adding...withLocalizePropTypesto thepropTypes, can we instead use theuseLocalizehook? - Bit odd that the displayName is different on native, but I guess that's fine.
|
Also, I don't think well need a C+ review here. |
|
@hannojg bump for changes and conflicts |
…f/remove-userdetailstooltip-mobile
…f/remove-userdetailstooltip-mobile
|
Hey @roryabraham sorry for the wait! This is good for review again! 😊 |
|
@roryabraham looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
Not an emergency, but I did actually forget to do the reviewer checklist here. My bad, but this is a trivial change where we just removed a tooltip on mobile, where it never shows anyways. Anyways, still should've completed the checklist... |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.39-3 🚀
|
|
@hannojg Can you pls clarify for QA team what is "SidebarLinks" component on the page |
|
@roryabraham @hannojg I'm unable to see changes of my PR #22567 after your merge |
|
Hey @chiragxarora good catch, so sorry, might have happened that I missed your change during merge. Double checking … |
|
Reapplied the changes in this PR: #22637 |
|
Heyyy @hannojg , can I make this PR instead of you? |
|
Hey, for sure! I also didn't meant to take the credits for the fix, sorry! its all yours, just wanted to help with creating the PR because I messed it up for you 😅 will close it, nw! |
|
I NEVER meant that @hannojg , I just love my name showing up on blame that's all😂😭 |
|
@chiragxarora do we have an ETA for this one? This would mean our original issue will be pushed in the payment timeline. |
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.39-11 🚀
|
| return ( | ||
| <Tooltip | ||
| renderTooltipContent={renderTooltipContent} | ||
| renderTooltipContentKey={[userDisplayName, userLogin]} |
There was a problem hiding this comment.
I think that was already there when i made the changes, so no clue, sorry
There was a problem hiding this comment.
Oops sorry my bad, the merge with main commit tricked me 😅. This was added in #22246.
Details
We only need the
UserDetailsTooltipcomponent on web, that's why we can skip it for the other platforms.Fixed Issues
$ #21796
PROPOSAL: #21796
Tests
Offline tests
Doesn't apply
QA Steps
Same as testing steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android