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

[HOLD for payment 2022-11-24] [$1000] Composer text and icon not aligned vertically centre for native app - reported by dhairyasenjaliya #10202

Closed
trjExpensify opened this issue Aug 2, 2022 · 117 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review

Comments

@trjExpensify
Copy link
Contributor

trjExpensify commented Aug 2, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Login in iOS app
  2. Go to any chat
  3. Type message in composer and notice the send icon and text written is aligned more to bottom rather than centre vertically

Expected Result:

Text should be proper aligned to vertically centre

Actual Result:

Text not aligned to vertically centre

Workaround:

unknown

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.1.86-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
textAlign
Screenshot_20220728-052649

Upwork job URL: https://www.upwork.com/jobs/~01b7d0ca259c9797ca
Issue reported by: dhairyasenjaliya @roryabraham
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1657875961211239
https://expensify.slack.com/archives/C01GTK53T8Q/p1659011710209859

View all open jobs on GitHub

@trjExpensify trjExpensify added Engineering Daily KSv2 Improvement Item broken or needs improvement. labels Aug 2, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2022

Current assignee @Julesssss is eligible for the Engineering assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@trjExpensify
Copy link
Contributor Author

For the avoidance of doubt, can we confirm in the OP for the contributor what the padding should be? it's 6px top and bottom isn't it? CC: @shawnborton

I'm also curious what PR broke this, I took a cursory look at recent ones and perhaps this?

@thesahindia
Copy link
Member

I took a cursory look at recent ones and perhaps this?

I don't think it's related because that PR is only related to edit composer.

@shawnborton
Copy link
Contributor

I agree that it's not vertically aligned. I think we want some default padding on top and bottom, but regardless, the text should be vertically aligned in the remaining space. There is a way to do this easily on Android I believe, but I'm not sure how we do it on iOS.

@roryabraham
Copy link
Contributor

Heads up! It looks like there are actually two (potentially separate) alignment issues at play here:

  1. Text misalignment

    image
  2. Icon misalignment

    image

@Julesssss Julesssss changed the title Composer text not aligned vertically centre for native app - reported by dhairyasenjaliya Composer text and icon not aligned vertically centre for native app - reported by dhairyasenjaliya Aug 3, 2022
@Julesssss
Copy link
Contributor

Thanks Rory. I recalled an existing issue for the icon, but can't find anything in our open issues 🤷

I updated the description and title to include both issues and will open up for contributors.

@Julesssss Julesssss added the External Added to denote the issue can be worked on by a contributor label Aug 3, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2022

Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 3, 2022
@trjExpensify
Copy link
Contributor Author

Upwork job here: https://www.upwork.com/jobs/~01cb430b907eadaa26

@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 3, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2022

Current assignee @Julesssss is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Composer text and icon not aligned vertically centre for native app - reported by dhairyasenjaliya [$250] Composer text and icon not aligned vertically centre for native app - reported by dhairyasenjaliya Aug 3, 2022
@swiftpipe
Copy link

Hi, This error occurs because the text input is set to multi-line mode, I've had this problem before

@swiftpipe
Copy link

swiftpipe commented Aug 3, 2022

Proposal
I think the problem may be because the line height is not correct so the text is not centered
Solution:
borderWidth: 0, borderRadius: 0, height: 'auto', lineHeight: 16,

simulator_screenshot_611360EB-B517-4B55-9EE1-A3CBEAD87F54

I tried it and it seems to work fine!

2022-08-03_20-24-06

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 3, 2022
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 11, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 11, 2022

📣 @fedirjh You have been assigned to this job by @Julesssss!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@trjExpensify
Copy link
Contributor Author

Awesome!! @fedirjh - I've sent you the job offer on Upwork! Please accept, and let us know the ETA of when we should expect a PR for this? 🚀

@fedirjh
Copy link
Contributor

fedirjh commented Nov 11, 2022

Awesome!! @fedirjh - I've sent you the job offer on Upwork! Please accept, and let us know the ETA of when we should expect a PR for this?

Thanks @trjExpensify , Offer accepted and #12669 created

@trjExpensify trjExpensify added the Reviewing Has a PR in review label Nov 11, 2022
@trjExpensify
Copy link
Contributor Author

Awesome! Over to you @Santhosh-Sellavel!

@Julesssss
Copy link
Contributor

The PR is reviewed and looks great. We just need to add some comments and create a new Test rail case to help prevent regressions.

@melvin-bot
Copy link

melvin-bot bot commented Nov 14, 2022

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

@Julesssss
Copy link
Contributor

I simply don't have time to locate a specific PR that introduced this, there are MULTIPLE regressions from weeks-months ago.

@fedirjh
Copy link
Contributor

fedirjh commented Nov 15, 2022

The PR that introduced the bug : #9031 and #1557

@trjExpensify
Copy link
Contributor Author

Yeah I was about to say @Julesssss, both of the offending PRs were referenced in this comment, I thought.

@Julesssss
Copy link
Contributor

Oh great, thanks. I didn't have a chance to review the comments 👍

@trjExpensify
Copy link
Contributor Author

Cool, completed 1-4 on the checklist. Will circle back once payment is due. Dropping to Weekly until then.

@trjExpensify trjExpensify added Weekly KSv2 and removed Daily KSv2 labels Nov 15, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 17, 2022
@melvin-bot melvin-bot bot changed the title [$1000] Composer text and icon not aligned vertically centre for native app - reported by dhairyasenjaliya [HOLD for payment 2022-11-24] [$1000] Composer text and icon not aligned vertically centre for native app - reported by dhairyasenjaliya Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.28-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-11-24. 🎊

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 23, 2022
@trjExpensify
Copy link
Contributor Author

Alrighty, payment time! @dhairyasenjaliya has been paid for reporting the issue.

@fedirjh & @Santhosh-Sellavel - 50% bonus for merging the PR within 3 days applies. @fedirjh, I've settled up. @Santhosh-Sellavel I've sent you an offer as you hadn't applied.

@trjExpensify
Copy link
Contributor Author

Cool, thanks @Santhosh-Sellavel. Settled!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests