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] FEATURE REQUEST: Allow navigating the list in the new group dialog with arrow keys #7648

Closed
mvtglobally opened this issue Feb 9, 2022 · 446 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2

Comments

@mvtglobally
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. Open app
  2. Navigate to "+" > "New Group"
  3. Type anyones name and add
  4. Use arrow keys to navigate

Expected Result:

User should be able to user arrow keys

Actual Result:

User is unable to use arrow keys

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Web
  • Desktop App

Version Number: 1.1.37-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
image - 2022-02-09T011717 191

Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644356006523069

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Feb 9, 2022
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Feb 9, 2022
@MelvinBot
Copy link

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

@lschurr lschurr removed their assignment Feb 9, 2022
@roryabraham
Copy link
Contributor

I have this working on a local branch but am currently scope-creeping and making arrow navigation work on all our OptionsLists, not just NewGroup. The one that's proving trickier is IOUConfirmationList.

@roryabraham
Copy link
Contributor

Just so you know I'm not just making stuff up, here's a draft PR with what I've got so far: #7702 🙃

@roryabraham
Copy link
Contributor

Got my PR done and almost ready for review. It's a bit of a beast so I want to make sure I write up super thorough test / QA steps.

@MelvinBot MelvinBot added Overdue and removed Overdue labels Feb 15, 2022
@MelvinBot
Copy link

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

@MelvinBot
Copy link

@roryabraham Still overdue 6 days?! Let's take care of this!

@roryabraham
Copy link
Contributor

Okay, ended up being a very big PR but I think there's a lot of good changes there and it's very thoroughly tested. Excited to see how the review goes.

@MelvinBot MelvinBot removed the Overdue label Feb 24, 2022
@roryabraham roryabraham added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 24, 2022
@mallenexpensify
Copy link
Contributor

Assigned to @rushatgabhane , he's reviewing and testing the PR, job price for doing so will be $500.
Rushat, please apply here https://www.upwork.com/jobs/~016a9b6f5ca7f87207

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

PR is being reviewed, it's a bit large :)

@roryabraham
Copy link
Contributor

Yeah, I'll try to push it forward ASAP. I might split it into multiple PRs to make it easier for the reviewers too. @rushatgabhane would it be helpful it I split off the KeyboardShortcut library changes from the rest?

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 9, 2022

@roryabraham participants being hidden is the main blocker for the PR.
If we can fix it, then there isn't much to gain by splitting the PR.

Splitting off KeyboardShortcut lib would be nice for the two tickets waiting on event bubbling to be implemented tho. (#7920 (comment), #7623)

@roryabraham
Copy link
Contributor

I'm sure we'll be able to fix the participants being hidden bug, but frankly this just isn't as high priority as some other issues I've got assigned to me at the moment. So I'm going to create a separate PR for the KeyboardShortcut lib upgrades then follow-up with this as soon as I can.

@melvin-bot
Copy link

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

11 similar comments
@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@melvin-bot
Copy link

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

@Expensify Expensify locked as spam and limited conversation to collaborators Aug 1, 2022
@Christinadobrzyn
Copy link
Contributor

Oh no! I'm not sure what I did here or if this is bad - @roryabraham would you be able to lend some insight?

@Christinadobrzyn
Copy link
Contributor

Not sure why this auto-message is posting so many times, asking eng - https://expensify.slack.com/archives/C03TQ48KC/p1659380948350629

@marcaaron
Copy link
Contributor

On it.

@Expensify Expensify unlocked this conversation Aug 1, 2022
@marcaaron
Copy link
Contributor

Sorry about that everyone. We accidentally created a bug when adding the new reminder feature. I think it should be resolved now. Going to leave a quick test comment here to confirm that can be ignored.

Test: #7648

@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2022

📣 @parasharrajat You have been assigned to this job by @Christinadobrzyn!
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 📖

@Christinadobrzyn Christinadobrzyn changed the title [$500] FEATURE REQUEST: Allow navigating the list in the new group dialog with arrow keys [$1000] FEATURE REQUEST: Allow navigating the list in the new group dialog with arrow keys Aug 3, 2022
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Aug 3, 2022

Please ignore the ping, Rajat, just adding some notes to this GH as the payment structure wasn't super clear.

  • Based on this convo here, @rushatgabhane should be compensated $1000 for the fix. Sounds like there was a regression so this should be $500.

  • Although Rajat wasn't ever officially the C+ he helped with the PR so we're going to compensate him as the C+.

  • It looks like there was 1 regression on this PR so the C+ compensation is $250.

Slack convo about this compensation here - https://expensify.slack.com/archives/C01SKUP7QR0/p1659537301960919

@rushatgabhane
Copy link
Member

Umm.. I thought I was alreaady paid $500.

I received $500 again 🤔

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Aug 3, 2022

Yes, based on this comment, you should have been paid $1000 - I didn't see that at the time of paying you for this job so I added the additional $500 as a bonus.

@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 3, 2022

@Christinadobrzyn If I'm not mistaken, base comp is $1000.

But there were regressions. So the total pay should be $500.

@Christinadobrzyn
Copy link
Contributor

Sounds good, I wasn't aware of the regressions, is it an option to reject the bonus?

@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 3, 2022

I'm not aware of any options to reject the bonus. But you can request a refund for it

@Christinadobrzyn
Copy link
Contributor

Sounds good, I just requested a refund @rushatgabhane

@parasharrajat I also created this new job to pay you the $250 - https://www.upwork.com/ab/applicants/1554909413932630016/job-details

Let me know when you've accepted the offer and I'll pay you.

@rushatgabhane
Copy link
Member

done!

@Christinadobrzyn
Copy link
Contributor

Paid @parasharrajat through this job posting - https://www.upwork.com/ab/applicants/1554909413932630016/job-details

I think we're all set here! Let me know if there's anything else.

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 NewFeature Something to build that is a new item. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants