Skip to content

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

Merged
tgolen merged 5 commits into
Expensify:mainfrom
sidferreira:sidferreira/issue_6096
Dec 10, 2021
Merged

Name and message aligned in iOS + Focus Mode - Issue 6096#6289
tgolen merged 5 commits into
Expensify:mainfrom
sidferreira:sidferreira/issue_6096

Conversation

@sidferreira

@sidferreira sidferreira commented Nov 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

sidferreira commented Nov 12, 2021

Copy link
Copy Markdown
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

tgolen commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

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

@sidferreira

sidferreira commented Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

@tgolen still not hired on upwork as well :D

@sidferreira sidferreira reopened this Nov 15, 2021
@sidferreira

sidferreira commented Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

closed by mistake (jumping ui)

@parasharrajat

parasharrajat commented Nov 15, 2021

Copy link
Copy Markdown
Member

I think the message is not correctly vertically aligned.

Cc @shawnborton .

@shawnborton

shawnborton commented Nov 16, 2021

Copy link
Copy Markdown
Contributor

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

@sidferreira

sidferreira commented Nov 16, 2021

Copy link
Copy Markdown
Contributor Author

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

@parasharrajat

parasharrajat commented Nov 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

@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

sidferreira commented Nov 18, 2021

Copy link
Copy Markdown
Contributor Author

@parasharrajat I think now it is working alright :)

@parasharrajat

parasharrajat commented Nov 18, 2021

Copy link
Copy Markdown
Member

Will check later? AFK

@sidferreira sidferreira mentioned this pull request Nov 22, 2021
Closed
@sidferreira

sidferreira commented Nov 25, 2021

Copy link
Copy Markdown
Contributor Author

@parasharrajat had the time to check this one?

@parasharrajat

parasharrajat commented Nov 25, 2021

Copy link
Copy Markdown
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
Comment thread src/styles/optionAlternateTextCompact/base.js Outdated
parasharrajat
parasharrajat suggested changes Nov 30, 2021
View reviewed changes
Comment thread src/styles/styles.js Outdated
Comment thread src/styles/optionAlternateTextCompact/index.ios.js Outdated
Comment thread src/styles/optionAlternateTextCompact/base.js Outdated
Comment thread src/styles/optionAlternateTextCompact/index.js Outdated
Comment thread src/styles/styles.js Outdated
parasharrajat
parasharrajat suggested changes Nov 30, 2021
View reviewed changes
Comment thread src/styles/optionAlternateTextCompact/index.ios.js Outdated
@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
Comment thread src/styles/optionAlternateTextPlatformStyles/index.js Outdated
@sidferreira @parasharrajat
Update src/styles/optionAlternateTextPlatformStyles/index.js
ee21d13 
Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>
tgolen
tgolen approved these changes Nov 30, 2021
View reviewed changes

@tgolen tgolen left a comment

Copy link
Copy Markdown
Contributor

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

sidferreira commented Dec 3, 2021

Copy link
Copy Markdown
Contributor Author

@tgolen are we waiting for anything?

@tgolen

tgolen commented Dec 3, 2021

Copy link
Copy Markdown
Contributor

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

parasharrajat
parasharrajat approved these changes Dec 3, 2021
View reviewed changes

@parasharrajat parasharrajat left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

parasharrajat commented Dec 10, 2021

Copy link
Copy Markdown
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

OSBotify commented Dec 10, 2021

Copy link
Copy Markdown
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

OSBotify commented Dec 10, 2021

Copy link
Copy Markdown
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

OSBotify commented Dec 20, 2021

Copy link
Copy Markdown
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

+1 more reviewer

@parasharrajat parasharrajat parasharrajat approved these changes

Reviewers whose approvals may not affect merge requirements

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