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-22] [$250] Routing number field doesn't get auto focused after opening the page reported by @Puneet-here #11423

Closed
kavimuru opened this issue Sep 29, 2022 · 66 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

Comments

@kavimuru
Copy link

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. Navigate to settings > workspace > connect bank account
  2. Connect manually

Expected Result:

Routing number field should get focused

Actual Result:

It doesn't get focused

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.2.9-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:

FMLR8413.1.MP4

Expensify/Expensify Issue URL:
Issue reported by: @Puneet-here
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1663687968571139

View all open jobs on GitHub

@kavimuru kavimuru added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Sep 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2022

Triggered auto assignment to @zanyrenney (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Sep 29, 2022
@Puneet-here
Copy link
Contributor

Proposal

We can use autoFocus here

@zanyrenney
Copy link
Contributor

zanyrenney commented Sep 29, 2022

I can't repro this, but from the video, it looks good for eng triage and the solution above seems like a good one!

@zanyrenney zanyrenney removed their assignment Sep 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2022

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

@iwiznia iwiznia removed their assignment Sep 29, 2022
@iwiznia iwiznia added the External Added to denote the issue can be worked on by a contributor label Sep 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 29, 2022

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

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

melvin-bot bot commented Sep 29, 2022

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

@melvin-bot melvin-bot bot changed the title Routing number field doesn't get auto focused after opening the page reported by @Puneet-here [$250] Routing number field doesn't get auto focused after opening the page reported by @Puneet-here Sep 29, 2022
@MitchExpensify
Copy link
Contributor

Upwork Job

@16NTU116
Copy link

Proposal

It can be solved by the following:

  1. Using autoFocus prop. Reference
  2. Use ref prop by setting it in componentDidMount or in useeffect

@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2022

@MonilBhavsar, @MitchExpensify, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Oct 3, 2022
@MonilBhavsar
Copy link
Contributor

@Santhosh-Sellavel could you please take a look at the proposals when you get chance

@melvin-bot melvin-bot bot removed the Overdue label Oct 3, 2022
@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Oct 3, 2022

@Puneet-here Can you add the behavior(recordings) of all platforms here after applying your proposal, thanks!

@Santhosh-Sellavel
Copy link
Collaborator

Also @MonilBhavsar Please confirm if the autofocus is expected here with the design/product team. Autofocus creates weird keyboard issues in native I believe intentionally we don't set autofocus, thanks!

@Santhosh-Sellavel
Copy link
Collaborator

@MitchExpensify or @MonilBhavsar
Due to Limited Availability unassigning this one. Can you reapply the external label to get another C+ assigned thanks!

@melvin-bot melvin-bot bot added the Overdue label Nov 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 10, 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:

@MonilBhavsar
Copy link
Contributor

The PR that introduced the bug has been identified. Link to the PR:

I think this was missed and not a regression by any PR. @rushatgabhane thoughts?

@melvin-bot melvin-bot bot removed the Overdue label Nov 10, 2022
@rushatgabhane
Copy link
Member

@MonilBhavsar agree. This isn't a regression

@MitchExpensify
Copy link
Contributor

Seeing as this wasn't a regression, I'm thinking that this step can be skipped in this case:

The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

I'm undecided if this still makes sense for the same reason:

. A discussion in #contributor-plus has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

Curious what you think @puneetlath ?

@puneetlath
Copy link
Contributor

In that case, I think the offending PR would be the one that created this feature.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Nov 15, 2022
@melvin-bot melvin-bot bot changed the title [$250] Routing number field doesn't get auto focused after opening the page reported by @Puneet-here [HOLD for payment 2022-11-22] [$250] Routing number field doesn't get auto focused after opening the page reported by @Puneet-here Nov 15, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 15, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.27-4 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-22. 🎊

@MitchExpensify
Copy link
Contributor

I think the offending PR would be the one that created this feature.

Thats fair. @MonilBhavsar / @rushatgabhane mind tracking that down and linking here?

@MitchExpensify
Copy link
Contributor

Made a note on my cal to pay on 11/22

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 22, 2022
@rushatgabhane
Copy link
Member

will get to this in 1-2 days. have a huge backlog

@MitchExpensify
Copy link
Contributor

Paid @Puneet-here for reporting bonus and fix

@rushatgabhane sent you an invite for C+ payment

@rushatgabhane
Copy link
Member

rushatgabhane commented Nov 23, 2022

@MitchExpensify the offer expired yesterday maybe

Orr it is accepted. I'm not sure

@MitchExpensify
Copy link
Contributor

@rushatgabhane All good! It is accepted. Payment made > Contract ended

@MonilBhavsar
Copy link
Contributor

Are we good to close this now?

@rushatgabhane
Copy link
Member

@MonilBhavsar yes good to close.
reporter, c and c+ all paid

@MonilBhavsar
Copy link
Contributor

Thanks! I see the the checklist needs still needs to be completed #11423 (comment)

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Nov 25, 2022

The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

Not a regression. May be It is like this since it was introduced.

A discussion in #contributor-plus has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

I think when we add a form component, PR author can start a discussion in slack if we want to autofocus field when we open the form. In this way we can prevent these kind of bugs.

cc @MitchExpensify

@melvin-bot
Copy link

melvin-bot bot commented Nov 28, 2022

@rushatgabhane, @MonilBhavsar, @MitchExpensify, @Puneet-here Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Nov 28, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

@rushatgabhane, @MonilBhavsar, @MitchExpensify, @Puneet-here Eep! 4 days overdue now. Issues have feelings too...

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Dec 1, 2022

Sorry I've been sick the last few days! Started the discussion about how to avoid this going forward here

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
Projects
None yet
Development

No branches or pull requests