Skip to content

Name and message aligned in iOS + Focus Mode - Issue 6096 #6289

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 10, 2021
Merged

Name and message aligned in iOS + Focus Mode - Issue 6096 #6289

merged 5 commits into from
Dec 10, 2021

Conversation

sidferreira
Copy link
Contributor

@sidferreira sidferreira commented Nov 12, 2021 •
edited
Loading

Details

On iOS + Focus Mode the message would not be aligned properly with the name if there was an emoji in the message.

FOCUS_IOS_NOT_FIXED

Fixed Issues

$ #6096

Tests

  1. Go to Preferences
  2. Change to Focus mode
  3. Be sure there's an emoji in the most recent message of the chat

QA Steps

...

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Platform Focus Mode Non Focus Mode
iOS Screen Shot 2021-11-18 at 3 03 40 PM Screen Shot 2021-11-18 at 3 03 57 PM
Android FOCUS_ANDROID_FOCUS FOCUS_ANDROID_NON_FOCUS
Desktop FOCUS_DESKTOP_FOCUS FOCUS_DESKTOP_NON_FOCUS
Web FOCUS_WEB_FOCUS FOCUS_WEB_NON_FOCUS
MWeb FOCUS_MWEB_FOCUS FOCUS_MWEB_NON_FOCUS

@sidferreira sidferreira requested a review from a team as a code owner November 12, 2021 23:34
@MelvinBot MelvinBot requested review from tgolen and removed request for a team November 12, 2021 23:34
@sidferreira
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

tgolen
tgolen previously approved these changes Nov 15, 2021
View reviewed changes
@tgolen
Copy link
Contributor

tgolen commented Nov 15, 2021

The original issue is still on hold, so I will wait on merging this until the hold is lifted.

@sidferreira
Copy link
Contributor Author

@tgolen still not hired on upwork as well :D

@sidferreira sidferreira reopened this Nov 15, 2021
@sidferreira
Copy link
Contributor Author

closed by mistake (jumping ui)

@parasharrajat
Copy link
Member

I think the message is not correctly vertically aligned.

Cc @shawnborton .

@shawnborton
Copy link
Contributor

Agree, I think we want the baseline of each font to be aligned.

@sidferreira
Copy link
Contributor Author

@shawnborton I'll look into it, but may be a bit trickier because of the emoji thing. Will update asap

@parasharrajat
Copy link
Member

parasharrajat commented Nov 18, 2021 •
edited
Loading

@sidferreira Please sign your commits. Unfortunately, it can't be merged without signing them.

@sidferreira sidferreira force-pushed the sidferreira/issue_6096 branch from c37873b to a7543de Compare November 18, 2021 17:34
@sidferreira sidferreira force-pushed the sidferreira/issue_6096 branch from a7543de to 8f3028e Compare November 18, 2021 18:03
@sidferreira
Copy link
Contributor Author

@parasharrajat I think now it is working alright :)

@parasharrajat
Copy link
Member

Will check later? AFK

@sidferreira sidferreira mentioned this pull request Nov 22, 2021
Closed
@sidferreira
Copy link
Contributor Author

@parasharrajat had the time to check this one?

@parasharrajat
Copy link
Member

Sorry, I am not actively tracking this one as this one is not on my checklist. But I will do a review today.

@sidferreira sidferreira requested a review from tgolen November 28, 2021 03:11
tgolen
tgolen requested changes Nov 30, 2021
View reviewed changes
parasharrajat
parasharrajat suggested changes Nov 30, 2021
View reviewed changes
parasharrajat
parasharrajat suggested changes Nov 30, 2021
View reviewed changes
@sidferreira sidferreira force-pushed the sidferreira/issue_6096 branch from 03d466e to 447e0eb Compare November 30, 2021 17:30
parasharrajat
parasharrajat suggested changes Nov 30, 2021
View reviewed changes
tgolen
tgolen approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Oh, I like this solution much better!

@sidferreira
Copy link
Contributor Author

@tgolen are we waiting for anything?

@tgolen
Copy link
Contributor

tgolen commented Dec 3, 2021

I was waiting for @parasharrajat to approve since he also reviewed the code.

parasharrajat
parasharrajat approved these changes Dec 3, 2021
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not tested on the platform but the code looks good. Sorry for keeping you waiting, I just reviewed to suggest improvement in the code.

@parasharrajat
Copy link
Member

@toglen. If you feel this is good. I think we can merge this.

@tgolen tgolen merged commit cc5e2d3 into Expensify:main Dec 10, 2021
@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.

@sidferreira sidferreira deleted the sidferreira/issue_6096 branch December 10, 2021 18:45
@OSBotify OSBotify mentioned this pull request Dec 10, 2021
Closed
38 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @tgolen in version: 1.1.19-5 🚀

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

@sidferreira sidferreira mentioned this pull request Dec 15, 2021
Merged
5 tasks
@NicMendonca NicMendonca mentioned this pull request Dec 16, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgolen tgolen tgolen approved these changes

@parasharrajat parasharrajat parasharrajat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sidferreira @tgolen @parasharrajat @shawnborton @OSBotify