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.

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

[Payment due May 31] [$2000] iPad - Login form position should be a little lower in tablets - reported by @thesahindia #8122

Closed
mvtglobally opened this issue Mar 14, 2022 · 59 comments
Closed

[Payment due May 31] [$2000] iPad - Login form position should be a little lower in tablets - reported by @thesahindia #8122

mvtglobally opened this issue Mar 14, 2022 · 59 comments
Assignees
@iwiznia @michaelhaxhiu @aneequeahmad @Santhosh-Sellavel
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@mvtglobally
Copy link

mvtglobally commented Mar 14, 2022 •
edited by michaelhaxhiu
Loading

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 :expensify: on tablet
  2. Go to the login page

Expected Result:

Login form position should be a little lower

Actual Result:

There's is too much gap

Workaround:

unknown

Platform:

Where is this issue occurring?

  • iOS

Version Number: 1.1.41-4
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
Screenshot 2022-02-26 at 2 29 00 AM
Screenshot 2022-02-26 at 2 17 27 AM
ipad-signin

Upwork job link - https://www.upwork.com/jobs/~0122e7f846be620b67
Issue reported by: @thesahindia
Slack conversation:
https://expensify.slack.com/archives/C01GTK53T8Q/p1645823447597149

View all open jobs on GitHub
@MelvinBot
Copy link

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

@flodnv
Copy link
Contributor

flodnv commented Mar 14, 2022

What do you propose the solution to be exactly @megankelso ? It's not 100% clear to me after reading the Slack thread.

@flodnv flodnv assigned megankelso and unassigned flodnv Mar 14, 2022
@megankelso
Copy link

This is the proposed solution!

ipad-signin (1)

@flodnv
Copy link
Contributor

flodnv commented Mar 14, 2022

Thanks! I find it a bit weird for some reason but it's alright :)

@flodnv flodnv added the External Added to denote the issue can be worked on by a contributor label Mar 14, 2022
@MelvinBot
Copy link

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

@megankelso
Copy link

Curious to know what you'r finding weird about it? I personally thought the graphic should be on top and the form on bottom, but with the keyboard coming up on the bottom of the screen I think this makes more sense.

@flodnv
Copy link
Contributor

flodnv commented Mar 14, 2022

I think both options are weird to me, because the screen is split in 2. I'm not a tablet user though, maybe if I was I wouldn't find it that weird.

@michaelhaxhiu
Copy link
Contributor

Okie so... seems like we feel good with the proposed mock, and I can post the job to upwork?

Fwiw, I think this version looks much better than the current version with the left/right content.

@mdneyazahmad
Copy link
Contributor

mdneyazahmad commented Mar 15, 2022 •
edited
Loading

I personally think the footer content should be at the very bottom and not the graphics. May be, we can completely remove the graphics (like mobile).

@michaelhaxhiu
Copy link
Contributor

Job post - https://www.upwork.com/jobs/~0122e7f846be620b67

@botify botify removed the Daily KSv2 label Mar 16, 2022
@MelvinBot MelvinBot added the Weekly KSv2 label Mar 16, 2022
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 16, 2022
@MelvinBot
Copy link

Triggered auto assignment to @iwiznia (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@michaelhaxhiu michaelhaxhiu changed the title iPad - Login form position should be a little lower in tablets - reported by @thesahindia [$500] iPad - Login form position should be a little lower in tablets - reported by @thesahindia Mar 24, 2022
@michaelhaxhiu
Copy link
Contributor

Doubling price to $500.

@MelvinBot MelvinBot removed the Overdue label Mar 24, 2022
@aneequeahmad
Copy link
Contributor

@Santhosh-Sellavel Submitted a new PR

@Santhosh-Sellavel
Copy link
Collaborator

Thanks!

@Santhosh-Sellavel
Copy link
Collaborator

PR is up review in progress!

@melvin-bot melvin-bot bot added the Overdue label May 9, 2022
@michaelhaxhiu
Copy link
Contributor

@aneequeahmad can you provide an update since it's been a little while here?

@melvin-bot melvin-bot bot removed the Overdue label May 9, 2022
@aneequeahmad
Copy link
Contributor

@michaelhaxhiu we had one edge case , when trying to resize the web/desktop app the UI was broken due to not enough height. Discussed with design team and added minimum height check for medium screens.

Pushed the changes and requested review on PR.

cc: @Santhosh-Sellavel

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented May 9, 2022 via email

@melvin-bot melvin-bot bot added the Overdue label May 17, 2022
@michaelhaxhiu
Copy link
Contributor

Updates on the PR as recent as yesterday, waiting for that to merge.

@melvin-bot melvin-bot bot removed the Overdue label May 18, 2022
@Julesssss
Copy link
Contributor

Hi All, a related issue was failing testing due to the login component being too high:

Screenshot 2022-05-23 at 12 11 43

I can reproduce the above and it looks like we decided that 854 was the trigger height:

  • How did we choose 854?
  • Shouldn't we be seeing the changes made in this PR on a 9.7 inch iPad Pro?

@Julesssss Julesssss mentioned this issue May 23, 2022
Closed
@aneequeahmad
Copy link
Contributor

@Julesssss Actually i decided the minimum height based on the point below which the height was too little to show graphics and login-layout-form here is my comment. Let me know what should i do. I think we should have a proper check to show the graphics and layout ? if so what it could be now thats the question.

@Santhosh-Sellavel
Copy link
Collaborator

@Julesssss I got it it's a typo issue left a comment here #8752 (comment)

@Julesssss Julesssss mentioned this issue May 23, 2022
Merged
@Julesssss
Copy link
Contributor

Raised a hotfix PR here: #9129

@Santhosh-Sellavel
Copy link
Collaborator

🤦
Oops this typo gonna cost me 1/2 of the payment?

@Julesssss
Copy link
Contributor

🤦 Oops this typo gonna cost me 1/2 of the payment?

Nah, don't worry. Obviously it would have been better to notice this during testing, but it happens and you helped me by pointing out the bug, so it's all good 🙂

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented May 27, 2022 •
edited
Loading

Just to confirm -- payment should be due on May 31s for this job, assuming no regressions on the PR, right?

@Santhosh-Sellavel
Copy link
Collaborator

@michaelhaxhiu
Can you set up a job for this one and hire me for C+ here, thanks!

@Julesssss
Copy link
Contributor

@michaelhaxhiu yep, looks like it deployed 6 days ago.

@michaelhaxhiu
Copy link
Contributor

@Santhosh-Sellavel - https://www.upwork.com/jobs/~01d1e846444a69ee31
@thesahindia - https://www.upwork.com/jobs/~01766fcad495ba2f4b

Please apply to this job

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented May 30, 2022 •
edited
Loading

Done. Thanks @michaelhaxhiu!

@michaelhaxhiu michaelhaxhiu changed the title [$2000] iPad - Login form position should be a little lower in tablets - reported by @thesahindia [Payment due May 31] [$2000] iPad - Login form position should be a little lower in tablets - reported by @thesahindia May 30, 2022
@thesahindia
Copy link
Member

Applied ✅

@michaelhaxhiu
Copy link
Contributor

All paid. Thanks for your efforts.

@thesahindia thesahindia mentioned this issue Aug 16, 2022
Closed
@melvin-bot
Copy link

melvin-bot bot commented Aug 16, 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.

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

@iwiznia iwiznia

@michaelhaxhiu michaelhaxhiu

@aneequeahmad aneequeahmad

@Santhosh-Sellavel Santhosh-Sellavel

Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests