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

Fix: Tooltip and message date position #3864

Merged
merged 9 commits into from
Jul 13, 2021
Merged

Fix: Tooltip and message date position #3864

merged 9 commits into from
Jul 13, 2021

Conversation

rdjuric
Copy link
Contributor

@rdjuric rdjuric commented Jul 2, 2021

Details

  1. Style changes to make the Tooltip responsive instead of fix width.
  2. Apply the Tooltip containerStyle in index.native.js for consistency across platforms
  3. Fixes the message timestamp to the same line as the displayName

Fixed Issues

Fixes an issue with the below PR where the Tooltip and the Timestamp of a message would me misplaced.
$ PR #3830

Tests

  1. Open any chat
  2. Check that the timestamp is line with the displayName
  3. Hoover over the displayName
  4. The Tooltip should show close to your cursor

QA Steps

Same as above.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web.mov

Mobile Web

mWeb.mp4

Desktop

desktop.mov

iOS

iOS.mp4

Android

video.mp4

@rdjuric rdjuric requested a review from a team as a code owner July 2, 2021 15:02
@MelvinBot MelvinBot requested review from tylerkaraszewski and removed request for a team July 2, 2021 15:02
@Luke9389 Luke9389 self-requested a review July 2, 2021 20:00
@Luke9389
Copy link
Contributor

Luke9389 commented Jul 2, 2021

@rdjuric in the future, with regressions, feel free to continue to tag and request reviews from engineers that are connected to the original issue

@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 2, 2021

@rdjuric in the future, with regressions, feel free to continue to tag and request reviews from engineers that are connected to the original issue

Makes sense! Will do that in the future @Luke9389

@rdjuric rdjuric requested a review from Luke9389 July 2, 2021 22:07
@Luke9389
Copy link
Contributor

Luke9389 commented Jul 5, 2021

linking this to the original issue: #3172

@rdjuric rdjuric requested a review from Luke9389 July 8, 2021 21:55
@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 8, 2021

Update made @Luke9389!

Changed our containerStyles to receive an array in our Tooltip and Hoverable to keep styling consistent across our code. Nice catch, thanks.

src/components/DisplayNames/index.js Show resolved Hide resolved
src/components/Tooltip/index.native.js Outdated Show resolved Hide resolved
src/components/Tooltip/index.native.js Show resolved Hide resolved
src/components/Tooltip/index.native.js Show resolved Hide resolved
@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 9, 2021

Made a few changes after your comments @parasharrajat. Thanks!

@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 11, 2021

Updated @Luke9389

@rdjuric rdjuric requested a review from Luke9389 July 11, 2021 01:03
Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more small things

src/components/Tooltip/index.native.js Show resolved Hide resolved
src/components/Tooltip/index.native.js Outdated Show resolved Hide resolved
@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 12, 2021

Updated @Luke9389

@rdjuric rdjuric requested a review from Luke9389 July 12, 2021 19:09
@Luke9389
Copy link
Contributor

OK, this is lookin' good. Can you be sure to retest on all 5 platforms with the most recent version of the code before we wrap this up?

@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 12, 2021

OK, this is lookin' good. Can you be sure to retest on all 5 platforms with the most recent version of the code before we wrap this up?

Will start retesting and tag you here once it's done!

@Luke9389
Copy link
Contributor

Thanks! (I know it's a pain but I've seen looooots of regressions caused by skipping this final step)

@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 13, 2021

@Luke9389

I tested this branch and the PR worked correctly. I merged it into our main branch locally just to make sure things were okay and looks like this commit 7729fc0 had a regression - it nullified the styling changes we made on the previous PR #3830.

That's why we have this Tooltip/message date bug on prod (where the avatar and the display name aren't wrapped in the Pressable) but not on staging (where they're wrapped in a Pressable)

To make our change work with the Pressable we need to move our flexShrink1 from the ToolTip to the Pressable component. The updated videos on the PR are from the latest commit, where I made this change.

I kept our containerStyle -> containerStyles changes even thought we're no longer passing any styling to the ToolTip because I think they make the code more consistent, but let me know if you think differently.

@Luke9389
Copy link
Contributor

OK, that sounds fine to me 👍

@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 13, 2021

Can we merge this? @Luke9389

@Luke9389 Luke9389 merged commit 362a323 into Expensify:main Jul 13, 2021
@Luke9389
Copy link
Contributor

was just testing on all platforms myself as well :)

@OSBotify
Copy link
Contributor

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

@rdjuric
Copy link
Contributor Author

rdjuric commented Jul 13, 2021

Thanks, @Luke9389! Sorry for missing this regression on the first PR.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.77-6🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants