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

iPad - IOU - Amount is shifted up and is not aligned with currency #3296

Closed
isagoico opened this issue Jun 2, 2021 · 41 comments
Closed

iPad - IOU - Amount is shifted up and is not aligned with currency #3296

isagoico opened this issue Jun 2, 2021 · 41 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor FirstPick Engineering only, please! Only add when there is an identified code solution. Improvement Item broken or needs improvement. Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Jun 2, 2021

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


Expected Result

The digit 0 must be in the same line to the currency sign

Actual Result

Digit 0 shifted up

Action Performed:

  1. Launch the app and Log in to applause.expensifail account
  2. On a conversation, click on the + button in the compose box
  3. Choose "Request Money"

Workaround:

No need, visual issue.

Platform:

Where is this issue occurring?

Web
iOS - iPad ✔️
Android
Desktop App
Mobile Web

Version Number: 1.0.60-0

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

Notes/Photos/Videos:

image

Expensify/Expensify Issue URL:

View all open jobs on Upwork

@MelvinBot
Copy link

Triggered auto assignment to @MariaHCD (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@MariaHCD
Copy link
Contributor

MariaHCD commented Jun 3, 2021

This looks like a good first pick. I believe we'll have to look in the IOUAmountPage component.

Just to clarify, @isagoico, the screenshot is from an iPad in the landscape orientation?

@MariaHCD MariaHCD added Weekly KSv2 FirstPick Engineering only, please! Only add when there is an identified code solution. and removed Daily KSv2 labels Jun 3, 2021
@MariaHCD MariaHCD removed their assignment Jun 3, 2021
@MariaHCD MariaHCD added the Improvement Item broken or needs improvement. label Jun 3, 2021
@isagoico
Copy link
Author

isagoico commented Jun 3, 2021

Hello! Yes, it's an ipad in landscape orientation.

@isagoico
Copy link
Author

isagoico commented Jun 7, 2021

Issue reproducible today during KI retests

2 similar comments
@isagoico
Copy link
Author

Issue reproducible today during KI retests

@isagoico
Copy link
Author

Issue reproducible today during KI retests

@iwiznia
Copy link
Contributor

iwiznia commented Jun 25, 2021

Not sure why this was not marked as external, doing that now.

@iwiznia iwiznia added the External Added to denote the issue can be worked on by a contributor label Jun 25, 2021
@MelvinBot
Copy link

Triggered auto assignment to @kevinksullivan (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 25, 2021
@parasharrajat
Copy link
Member

parasharrajat commented Jun 25, 2021

There is another issue with currency input. There is no limit of input that can be entered.

issue.mp4

@isagoico
Copy link
Author

Issue reproducible during KI retests.

1 similar comment
@isagoico
Copy link
Author

isagoico commented Jul 5, 2021

Issue reproducible during KI retests.

@rdjuric
Copy link
Contributor

rdjuric commented Jul 5, 2021

Is this issue still available @kevinksullivan? Looks like it has the same root cause as #3849

  1. Since TextInput doesn't grow by default, we copy what we type in it to a hidden Text component, and use the width of that Text in our TextInput. We do this in our TextInputAutoWidth.
  2. To hide the Text component, we use this styling:
    https://github.com/Expensify/Expensify.cash/blob/045b39417e2da322680a4dc931830f970a1c46ed/src/styles/styles.js#L1589-L1595

Our problem is that position: 'relative' and transform: 'translateX(-100%)' are not valid styles in RN, only on the web. This gives us this difference (i just added some opacity so we can see the hidden text):

iPad Web
iPad

So in iPad we're actually rendering the hidden Text together with out TextInput, causing the bump we see.

Proposal

I think the best way for us to solve this is by changing our styling to use position: 'absolute' and moving our hidden Text outside of the View that wraps our TextInput. I don't think the transform is still useful, but if we want to keep it we have to pass the width of our view as RN doesn't support percentage based transforms.

@parasharrajat
Copy link
Member

parasharrajat commented Jul 7, 2021

Proposal

  1. I just tested it on Ipad pro but it is showing wrong input here. Instead of the current input that was made for web/desktop devices with a keyboard. It should show our on-screen keypad as this is again a native device.

  2. This happens as the this.isSmallScreenWidth variable is set to windowWidth <= 800 which is false for Ipad pro. Ipad can have a higher resolution like 1024px wide.

  3. To fix I will create another state varaible in withWindowDimensions HOC, named something like isMediumScreenWidth which would be windowWith <= 1200 && >== 800.

  4. By using this we will show the on-screen keypad on the AmountPage and it should fix this issue at the base.

cc: @shawnborton Do we want to show the on-screen Keypad on IPad devices or it should be the text input that triggers the software keyboard?

If we want this behavior then it will solve a couple of other issues related to the Amount Page on Ipads well.

@shawnborton
Copy link
Contributor

I'm curious for @roryabraham 's input as you had some thoughts regarding iPad/native device detection in another issue.

I think for iPad (and basically any touch-screen device?) it makes sense to show the custom number input we created for this screen.

@parasharrajat
Copy link
Member

Ok. For Touch screens, we already have CanUseTouchScreens which also covers the laptops with touch screens.

@johnmlee101
Copy link
Contributor

@rdjuric Sounds good! Let's go with @roryabraham 's suggestion and make sure its cross-platform as opposed to platform agnostic.

@kevinksullivan
Copy link
Contributor

@rdjuric want to submit a proposal so I can hire you for this?

@rdjuric
Copy link
Contributor

rdjuric commented Jul 23, 2021

@rdjuric want to submit a proposal so I can hire you for this?

I already submited a proposal in the job you linked! Not sure if you can see it on this link, but here it is

@isagoico
Copy link
Author

Issue not reproducible during KI retests. (Fixed 🎉)

@kevinksullivan
Copy link
Contributor

Waiting till 7 days after merge and then will make the payment.

@MelvinBot MelvinBot added Overdue and removed Overdue labels Jul 26, 2021
@MelvinBot
Copy link

@johnmlee101, @kevinksullivan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@roryabraham roryabraham added Weekly KSv2 and removed Daily KSv2 labels Jul 30, 2021
@MelvinBot MelvinBot removed the Overdue label Jul 30, 2021
@rdjuric
Copy link
Contributor

rdjuric commented Jul 30, 2021

Any updates here @kevinksullivan?

@isagoico
Copy link
Author

isagoico commented Aug 1, 2021

@rdjuric A tester was able to reproduce this during KI retests. Asking other testers with iPad to check if we're able to gather more info on the reproduction.
image

@rdjuric
Copy link
Contributor

rdjuric commented Aug 2, 2021

Thanks, @isagoico!

This a regression from another PR, where the changes I made to hiddenElementOutsideOfWindow were undone by mistake. VScode points me to this commit as the first time the changes were undone.

@parasharrajat
Copy link
Member

Ok. looks like I did that commit. @rdjuric Could you please tell me what were the changes? Here is the PR for that Commit. 5e159ff.

@rdjuric
Copy link
Contributor

rdjuric commented Aug 2, 2021

Here you go @parasharrajat! I think there were changes to TextInputAutoWidth too

@parasharrajat
Copy link
Member

but if you look at the commit changes. I have not touched any files related to your changes.

@parasharrajat
Copy link
Member

@rdjuric Could you please confirm?

@rdjuric
Copy link
Contributor

rdjuric commented Aug 2, 2021

@parasharrajat Yes, that's confusing me a bit! Still, when I try to git bisect the first time I see the file being changed is on that commit I linked.

Really weird. Let me know if you think the regression was caused by another PR.

@parasharrajat
Copy link
Member

ok @rdjuric. I think it's best if you redo those changes as a PR. I have not an idea of what needs to be done. Sorry for the inconvenience caused if I overwrote your changes.

@mallenexpensify
Copy link
Contributor

@rdjuric It does look like you weren't paid in the 7-day window but there's also been a regression. I'd prefer to wait for the fix til payment, is that alright with you?

@rdjuric
Copy link
Contributor

rdjuric commented Aug 3, 2021

No worries @parasharrajat I agree it's easier for me to re-do the PR, doing it now!

@mallenexpensify That's okay! I was already paid on Upwork. Just a note that the regression wasn't caused by this PR - the changes made here were removed in another one by mistake. I've already opened a PR to reapply the removed changes 👍

@MelvinBot

This comment has been minimized.

@mallenexpensify
Copy link
Contributor

@rdjuric should this issue be closed or wait for the new PR to be merged first?

@rdjuric
Copy link
Contributor

rdjuric commented Aug 4, 2021

@mallenexpensify This can be closed now that the new PR was merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor FirstPick Engineering only, please! Only add when there is an identified code solution. Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

No branches or pull requests