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

[$1000] Android - Amount digit in send money page is not aligned with currency text reported by @aneequeahmad in (#11952) #13309

Closed
kbecciv opened this issue Dec 3, 2022 · 49 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 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Dec 3, 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. Open app
  2. Click on + icon (green) > split bill or request money
  3. Enter any digit

Expected Result:

Amount digit should be aligned with currency text

Actual Result:

Amount digit in send money page is not aligned with currency text

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Android

Version Number: 1.2.36.0

Reproducible in staging?: Yes

Reproducible in production?: n/a

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5846973_az_recorder_20221203_094252.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Issue reported by: @aneequeahmad in this issue #11952
Slack conversation: expensify.slack.com/archives/C01GTK53T8Q/p1665613033013319

View all open jobs on GitHub

Upwork Automation - Do Not Edit

@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 3, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 3, 2022

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@markarmanus
Copy link
Contributor

Cant seem to recreate the issu, can you confirm its still happening on staging ?

@kadiealexander
Copy link
Contributor

This shows correctly on a Galaxy A8 device:

image

@kbecciv can you please add your device make and model to the post, and update your reproduction steps? Thanks.

@kbecciv
Copy link
Author

kbecciv commented Dec 5, 2022

@kadiealexander Issue was reproduced in Samsung Galaxy A32 / 12

@kadiealexander
Copy link
Contributor

kadiealexander commented Dec 6, 2022

Okay, tested on a few Samsung devices and was able to find one that replicated the error (Samsung Galaxy A52). This is a serious edge case, though.

image

@kadiealexander
Copy link
Contributor

From this SO: https://stackoverflow.com/c/expensify/questions/14418

  • The "bug" is actually a bug: Yes
  • The bug is not a duplicate report of an existing GH (close the GH and add any novel details to the original GH instead): Not a dupe.
  • The bug is reproducible, following the reproduction steps: [$1000] Android - Amount digit in send money page is not aligned with currency text reported by @aneequeahmad in (#11952) #13309 (comment)
  • If you’re unable to reproduce the bug, add the Needs reproduction label. Comment on the issue outlining the steps you took to try to reproduce the bug, your results and tag the issue reporter and the Applause QA member who created the issue. Ask them to clarify reproduction steps and/or to check the reproduction steps again. Close issue.:
  • The GH template is filled out as fully as possible -- this means the GH body and title are clear (ie. could an external contributor understand it and work on it?):
  • The GH was created by an Expensify employee or a QA tester:
  • If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it’s better to wait, close the GH & provide this justification:
  • If there's a link to Slack, check the discussion to see if we decided not to fix it:
  • Decide if the GH should be resolved by an External contributor or Internal engineer, add the appropriate label:

@kadiealexander kadiealexander added the External Added to denote the issue can be worked on by a contributor label Dec 6, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 6, 2022

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

@kadiealexander kadiealexander reopened this Dec 6, 2022
@melvin-bot melvin-bot bot changed the title Android - Amount digit in send money page is not aligned with currency text [$1000] Android - Amount digit in send money page is not aligned with currency text Dec 6, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 6, 2022

Job added to Upwork: https://www.upwork.com/jobs/~01bf6f429dad16a353

@melvin-bot
Copy link

melvin-bot bot commented Dec 6, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

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

melvin-bot bot commented Dec 6, 2022

Triggered auto assignment to @youssef-lr (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@markarmanus
Copy link
Contributor

@kadiealexander What is this tool you are using to test on different devices in the screenshot ?

@kadiealexander
Copy link
Contributor

We use a tool called Browserstack. :)

@fedirjh
Copy link
Contributor

fedirjh commented Dec 7, 2022

Proposal

we have to add lineHeight to iouAmountText , this will ensure that iouAmountText and iouAmountTextInput will have the same height

App/src/styles/styles.js

Lines 2138 to 2143 in 736359b

iouAmountText: {
fontFamily: fontFamily.EXP_NEUE_BOLD,
fontWeight: fontWeightBold,
fontSize: variables.iouAmountTextSize,
color: themeColors.heading,
},

 iouAmountText: { 
     fontFamily: fontFamily.EXP_NEUE_BOLD, 
     fontWeight: fontWeightBold, 
     fontSize: variables.iouAmountTextSize, 
     color: themeColors.heading, 
+    lineHeight: variables.inputHeight,
 },

@nishancx
Copy link
Contributor

nishancx commented Dec 7, 2022

Proposal
Issue:
The issue was caused when border of TextInput was removed with styles.borderNone here:

textInputContainerStyles={[styles.borderNone, styles.noLeftBorderRadius, styles.noRightBorderRadius]}

Solution:
Add padding: 1 to textInputContainerStyles prop in the above line

Output:
image

@lordthien
Copy link

lordthien commented Dec 7, 2022

Proposal
in file App/src/styles/styles.js
Add textAlign to iouAmountText, this ensures that choose any currency in the world will be aligned for all device.

    iouAmountText: {
        fontFamily: fontFamily.EXP_NEUE_BOLD,
        fontWeight: fontWeightBold,
        fontSize: variables.iouAmountTextSize,
        color: themeColors.heading,
+       textAlign: "center",
    },

@PauloGasparSv PauloGasparSv removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 7, 2022
@PauloGasparSv
Copy link
Contributor

@kbecciv you also added some screenshots where the order of the currency and amount are inverted like here:

Does that still happen to you and in what devices? Also, if it still happens do we have another issue for it or can we create another? Because it seems like a different problem than the one reproduced here about the alignment.

@PauloGasparSv
Copy link
Contributor

I had a hard time finding an emulator or device that reproduces the issue.

Other than the proposed changes, I was also thinking of using the textAlignVertical Android-only style property.

Since I can't reproduce the error, I did look at the proposals but can't approve any yet.
I think that @lordthien's proposal will align items on the horizontal axis instead of the vertical right?

@lordthien @nishancx @fedirjh did any of you manage to reproduce the problem locally before applying the fix?

@kadiealexander kadiealexander removed their assignment Dec 20, 2022
@kadiealexander kadiealexander added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Dec 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 20, 2022

Triggered auto assignment to @isabelastisser (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 20, 2022
@kadiealexander
Copy link
Contributor

Reassigning as I'm heading OOO for a family emergency. Thanks for the help @isabelastisser!!

@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Dec 22, 2022
@isabelastisser
Copy link
Contributor

Update: @fedirjh and @rushatgabhane were paid, and their contract ended.

@aneequeahmad, I invited you to the job in Upwork yesterday. Please accept the offer, and I will hire you and issue the payment as soon as possible. Thanks!

@aneequeahmad
Copy link
Contributor

@isabelastisser I have accepted the offer. Thanks

@mallenexpensify mallenexpensify self-assigned this Dec 24, 2022
@mallenexpensify
Copy link
Contributor

@aneequeahmad is paid $250 for reporting, thanks!
@isabelastisser I co-assigned, one of us needs to do the regression step tests, I'll likely get to next week cuz I'm around all week.

@mallenexpensify mallenexpensify changed the title [HOLD for payment 2022-12-22] [$1000] Android - Amount digit in send money page is not aligned with currency text reported by @aneequeahmad in (#11952) [Regression steps needed] [$1000] Android - Amount digit in send money page is not aligned with currency text reported by @aneequeahmad in (#11952) Dec 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jan 2, 2023

@PauloGasparSv, @mallenexpensify, @rushatgabhane, @isabelastisser, @fedirjh Huh... This is 4 days overdue. Who can take care of this?

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Jan 3, 2023
@isabelastisser
Copy link
Contributor

Update: Regression test buddy check is posted here.

@isabelastisser isabelastisser changed the title [Regression steps needed] [$1000] Android - Amount digit in send money page is not aligned with currency text reported by @aneequeahmad in (#11952) [$1000] Android - Amount digit in send money page is not aligned with currency text reported by @aneequeahmad in (#11952) Jan 6, 2023
@isabelastisser
Copy link
Contributor

@mallenexpensify can we close this?

@mallenexpensify
Copy link
Contributor

The two things still needed

  1. For the QA GH to be created to update the test steps in TestRail and for the link to be added here
  2. For @PauloGasparSv and/or @rushatgabhane to complete the steps here

@isabelastisser
Copy link
Contributor

Thanks, Matt!

QA GH was created last week, waiting for updates in TestRail.

@PauloGasparSv
Copy link
Contributor

PauloGasparSv commented Jan 9, 2023

@mallenexpensify filled the steps*! Sorry for the delay.

But I don't think this style bug is worthy of a Expensify open source discussion or commenting on the original issue as it was a tiny almost 1px misalignment that I think would be almost impossible to find during tests. Is that ok or should I do those anyway?

(EDIT: I tagged the wrong Matt here yesterday facepalm)

@mallenexpensify mallenexpensify added Daily KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels Jan 20, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 20, 2023
@melvin-bot melvin-bot bot removed the Overdue label Jan 20, 2023
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 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests