fix chat-link in end of line displays tooltip over text and not on link #29134
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
gedu
reviewed
Oct 10, 2023
This comment was marked as off-topic.
This comment was marked as off-topic.
pecanoro
requested review from
pecanoro and
s77rt
and removed request for
Li357
s77rt
suggested changes
Oct 10, 2023
|
@ 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] |
|
Recheck |
s77rt
suggested changes
Oct 11, 2023
|
@s77rt It's all resolved, you can recheck. Thanks. |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - ChromeMobile Web - SafariDesktopdesktop.moviOSAndroid |
s77rt
approved these changes
Oct 11, 2023
|
@teneeto Thanks! |
pecanoro
approved these changes
Oct 12, 2023
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
6 tasks
tgolen
reviewed
Oct 12, 2023
| // multiple lines, we want to show the tooltip over the part | ||
| // of the link that the user is hovering over. | ||
| const betterBounds = chooseBoundingBox(target.current, initialMousePosition.current.x, initialMousePosition.current.y); | ||
| setWrapperWidth(betterBounds.width); |
There was a problem hiding this comment.
I believe this is leading to a crash with Uncaught TypeError: Cannot read properties of undefined (reading 'width'). This is what I did:
- Sign in with a new account
- Go to Settings > Workspaces > Create New Workspace (don't create anything)
- Click the back button on the modal
There was a problem hiding this comment.
Yes it's, more info #29446 (comment) and PR fixing it #29468
97 tasks
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.3.84-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.3.85-0 🚀
|
6 tasks
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀
|
s77rt
reviewed
Oct 23, 2023
| const initialMousePosition = useRef({x: 0, y: 0}); | ||
|
|
||
| const updateTargetAndMousePosition = useCallback((e) => { | ||
| target.current = e.target; |
There was a problem hiding this comment.
We should have used the currentTarget (the element that is listening to the mouse enter event) instead of target (the element that caused the event to fire and bubble up). Coming from #29678
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
This PR seeks to address the reviews of a previously authored and closed PR
Referencing the leading solution #27817 (comment)
Add a new prop to Tooltip called shouldUseMultilinePositioning. This enables a new algorithm for finding the bounding box target for the tooltip. In the case where the target has wrapped onto multiple lines (e.g. it is a link in a chat window) then the link has multiple bounding boxes, and we want to show the tooltip against the one that the user is hovering over.
As part of this, extend Hoverable to pass the Event to its onHoverIn / onHoverOut callbacks.
Enable this new algorithm from BaseAnchorForCommentsOnly, which is the base class for links that show up in chat.
Fixed Issues
$ #27585
PROPOSAL: #27585
Tests
Referencing the leading solution #27817 (comment)
Action Performed:
On a platform with a mouse (i.e. not mobile):
Open the app
Open any report
Type in some text and at the end of line, add link with long text eg: Test test test test test test test test test test
Send the text, text should have text and link in first line and link should be continued in second line too. Adjust the window size to force the text to wrap if necessary.
Hover on link and observe that tooltip is displayed over text
Expected Result:
App should display tooltip over link on hover. The tooltip should appear over the same section of the link that the user is hovering over.
Offline tests
QA 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
Screen.Recording.2023-10-10.at.01.14.37.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android