[Fix]: URL isn't fully visible in the tooltip #12408

fedirjh · 2022-11-02T22:47:51Z

Details

fix: Tooltip wrap issue on Safari #10504 caused an issue in Desktop / Web preventing long URLs from breaking text to new line
Fixed wrong width calculation that leads to This issue : [HOLD for payment 2022-09-02] [$250] The unpin tooltip is clipped on Desktop #9331

Fixed Issues

$ #11486
PROPOSAL: #11486 (comment)

Tests & QA Steps

test 1 : Long URL

Navigate to any chat
Send a long URL : select from list

Hover
The whole Url is visible

Test 2 : Pin / Upin

Open any chat
Hover over "Pin" icon and press it.
The Unpin tooltip should be in one line without being trimmed/wrapped

Test 3 : Copy Email/Phone no.

Open Desktop Website/App
Navigate to Chat > Details
Copy Email/Phone no.
The Copy to clipboard text should be in one line without being trimmed/wrapped

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Web - Safari
https://user-images.githubusercontent.com/36869046/199615934-27c3d7c3-7147-4ee5-be6b-100e975f6e30.mov

Web - Chrome
https://user-images.githubusercontent.com/36869046/199616069-6d1b869b-935c-4e37-9560-7836a98c2855.mov

Web - Firefox
https://user-images.githubusercontent.com/36869046/199616362-a9760aa6-7c18-4212-8671-0cdb8264e4c1.mov

Mobile Web - Chrome

mWeb.Chrome.webm

Mobile Web - Safari

Simulator.Screen.Recording.-.iPhone.13.-.2022-11-03.at.12.29.00.mp4

Desktop

w_115226f14fcd6641e5b83fef04d6916d816dca77.mov

iOS

Native.-.IOS.mov

Android

Native.-.Android.mov

sobitneupane

@fedirjh Please Add screenshots for native and mWeb as well. We just want to make sure that the changes made here do not crash them.

Please add test steps for tooltips of Pin/Unpin and Copy to clipboard/copied as well.

fedirjh · 2022-11-03T06:22:06Z

@fedirjh Please Add screenshots for native and mWeb as well. We just want to make sure that the changes made here do not crash them.

Please add test steps for tooltips of Pin/Unpin and Copy to clipboard/copied as well.

@sobitneupane Tooltip doesn't work on mobile or native
#10504 (comment)

App/src/components/Tooltip/index.native.js

Lines 3 to 4 in dfb5bca

    
           // We can't use the common component for the Tooltip as Web implementation uses DOM specific method to 
        
           // render the View which is not present on the Mobile.

sobitneupane · 2022-11-03T06:27:08Z

Tooltip doesn't work on mobile or native

@fedirjh Yes, You are right. We just want to make sure that changes do not crash or produce any console errors in native app/ mWeb.

We just want to make sure that the changes made here do not crash them.

fedirjh · 2022-11-03T13:52:11Z

@sobitneupane can you request review from @thienlnam too ?

sobitneupane · 2022-11-03T13:57:48Z

@fedirjh Can you please add those native and mWeb Screenshots in PR body.

Also you might have missed the below comment. Please update test steps in PR body to address the following.

Please add test steps for tooltips of Pin/Unpin and Copy to clipboard/copied as well.

@sobitneupane can you request review from @thienlnam too ?

It's a known issue (On re-requesting review, other reviewer is being removed). But I cannot request review. Once done with my review. I will let him know.

fedirjh · 2022-11-03T14:26:03Z

@fedirjh Can you please add those native and mWeb Screenshots in PR body.

Please add test steps for tooltips of Pin/Unpin and Copy to clipboard/copied as well.

@sobitneupane done .

It's a known issue (On re-requesting review, other reviewer is being removed). But I cannot request review. Once done with my review. I will let him know.

Alright

sobitneupane

@thienlnam Changes look good and test well on all platforms.
🎀👀🎀 C+ reviewed

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

Untitled.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

Screen.Recording.2022-11-04.at.21.27.55.mov

iOS

Android

thienlnam · 2022-11-04T18:09:34Z

src/components/Tooltip/TooltipRenderedOnPageBody.js

+
+        // Reset the tooltip text width to 0 so that we can measure it again.
+        // eslint-disable-next-line react/no-did-update-set-state
+        this.setState({tooltipTextWidth: 0}, this.updateTooltipTextWidth);


This lint is correct - the placement of this is not ideal as we're adding infinite re-renders here
Can you find a better placement to reset the tooltip text width?

if (prevProps.text === this.props.text) { return; }

this line will prevent the infinite re-rendering , we are only updating tooltipTextWidth , once , when text changes

ah good point, missed that

thienlnam · 2022-11-04T18:56:54Z

src/components/Tooltip/TooltipRenderedOnPageBody.js

+
+        // Reset the tooltip text width to 0 so that we can measure it again.
+        // eslint-disable-next-line react/no-did-update-set-state
+        this.setState({tooltipTextWidth: 0}, this.updateTooltipTextWidth);


ah good point, missed that

melvin-bot · 2022-11-04T18:57:10Z

@thienlnam looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-11-04T19:02:15Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

mountiny · 2022-11-08T15:17:29Z

Just testing something here.

OSBotify · 2022-11-08T19:39:15Z

🚀 Deployed to staging by @thienlnam in version: 1.2.25-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-11-09T21:14:17Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.25-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[Fix]: URL isn't fully visible in the tooltip

334ea57

fedirjh requested a review from a team as a code owner November 2, 2022 22:47

melvin-bot bot requested review from sobitneupane and thienlnam and removed request for a team November 2, 2022 22:47

sobitneupane reviewed Nov 3, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

fedirjh requested review from sobitneupane and removed request for thienlnam November 3, 2022 13:45

sobitneupane approved these changes Nov 4, 2022

View reviewed changes

thienlnam requested changes Nov 4, 2022

View reviewed changes

thienlnam approved these changes Nov 4, 2022

View reviewed changes

thienlnam merged commit d6bac25 into Expensify:main Nov 4, 2022

melvin-bot bot added the Emergency label Nov 4, 2022

thienlnam removed the Emergency label Nov 4, 2022

mountiny added the FirstPick Engineering only, please! Only add when there is an identified code solution. label Nov 8, 2022

mountiny removed the FirstPick Engineering only, please! Only add when there is an identified code solution. label Nov 8, 2022

OSBotify mentioned this pull request Nov 8, 2022

Deploy Checklist: New Expensify 2022-11-08 #12563

Closed

34 tasks

melvin-bot bot mentioned this pull request Nov 9, 2022

[HOLD for payment 2022-11-16] [$500] [BUG] - Url isn't fully visible in the tooltip reported by @thesahindia #11486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix]: URL isn't fully visible in the tooltip #12408

[Fix]: URL isn't fully visible in the tooltip #12408

fedirjh commented Nov 2, 2022 •

edited

Loading

sobitneupane left a comment

fedirjh commented Nov 3, 2022 •

edited

Loading

sobitneupane commented Nov 3, 2022 •

edited

Loading

This comment was marked as resolved.

fedirjh commented Nov 3, 2022

sobitneupane commented Nov 3, 2022 •

edited

Loading

fedirjh commented Nov 3, 2022 •

edited

Loading

sobitneupane left a comment

thienlnam Nov 4, 2022

fedirjh Nov 4, 2022

thienlnam Nov 4, 2022

thienlnam Nov 4, 2022

melvin-bot bot commented Nov 4, 2022

OSBotify commented Nov 4, 2022

mountiny commented Nov 8, 2022

OSBotify commented Nov 8, 2022

OSBotify commented Nov 9, 2022

[Fix]: URL isn't fully visible in the tooltip #12408

[Fix]: URL isn't fully visible in the tooltip #12408

Conversation

fedirjh commented Nov 2, 2022 • edited Loading

Details

Fixed Issues

Tests & QA Steps

test 1 : Long URL

Test 2 : Pin / Upin

Test 3 : Copy Email/Phone no.

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

sobitneupane left a comment

Choose a reason for hiding this comment

fedirjh commented Nov 3, 2022 • edited Loading

sobitneupane commented Nov 3, 2022 • edited Loading

This comment was marked as resolved.

fedirjh commented Nov 3, 2022

sobitneupane commented Nov 3, 2022 • edited Loading

fedirjh commented Nov 3, 2022 • edited Loading

sobitneupane left a comment

Choose a reason for hiding this comment

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

thienlnam Nov 4, 2022

Choose a reason for hiding this comment

fedirjh Nov 4, 2022

Choose a reason for hiding this comment

thienlnam Nov 4, 2022

Choose a reason for hiding this comment

thienlnam Nov 4, 2022

Choose a reason for hiding this comment

melvin-bot bot commented Nov 4, 2022

OSBotify commented Nov 4, 2022

mountiny commented Nov 8, 2022

OSBotify commented Nov 8, 2022

OSBotify commented Nov 9, 2022

fedirjh commented Nov 2, 2022 •

edited

Loading

fedirjh commented Nov 3, 2022 •

edited

Loading

sobitneupane commented Nov 3, 2022 •

edited

Loading

sobitneupane commented Nov 3, 2022 •

edited

Loading

fedirjh commented Nov 3, 2022 •

edited

Loading