Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$500] Android - Workspace - Unable to select members in Workspace to invite members by tapping ->key #27371

Closed lanitochka17 closed 1 year ago

lanitochka17 commented 1 year ago

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. Go to https://staging.new.expensify.com/
  2. Tap fab
  3. Tap split bill
  4. Enter an amount
  5. Tap next
  6. Tap ->key repeatedly to select contacts
  7. Tap profile icon
  8. Tap Workspaces
  9. Tap any Workspace
  10. Tap members
  11. Tap invite
  12. Tap ->key repeatedly to select contacts
  13. Enter an email and try to tap ->key repeatedly to select contacts

Expected Result:

Tapping ->key to select contacts in split bill is working. Similarly, user with same key must be able to select members in Workspace to invite members

Actual Result:

Tapping ->key to select contacts in split bill is working but with same key unable to select members in Workspace to invite members

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.69-0

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/78819774/720011a9-4376-4215-bd22-7340843b1d18

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013b10c09c059cb764
  • Upwork Job ID: 1702332070742261760
  • Last Price Increase: 2023-09-21
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

dukenv0307 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Android - Workspace - Unable to select members in Workspace to invite members by tapping ->key

What is the root cause of that problem?

if we don't pass the initiallyFocusedOptionKey prop, the default focusedIndex state will be -1. So it's doesn't work when we tap -> key or enter in web

https://github.com/Expensify/App/blob/d494433a80ff82f588e3888a8da2c527848843e9/src/components/SelectionList/BaseSelectionList.js#L139

What changes do you think we should make in order to solve the problem?

I think we should set the default focusedIndex as 0 if initiallyFocusedOptionKey prop is undefined to fix for all pages that are using SectionList and make this component consists with OptionSelector

What alternative solutions did you explore? (Optional)

If we don't want all pages have this behavior, we can add an extra prop to decide set default focus or not or we can pass initiallyFocusedOptionKey as 0 for the special pass that we want to have the default focus to use the key to select

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~013b10c09c059cb764

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

ntdiary commented 1 year ago
image

@dukenv0307, it looks like split page also does not pass initiallyFocusedOptionKey, why can it work well?

dukenv0307 commented 1 year ago

@ntdiary This page use OptionSelector and if we don't pass the focus prop, the default focusIndex will be 0.

ntdiary commented 1 year ago

@dukenv0307, ah, got it, thank you! By the way, this seems to be the expected behavior for issue #25890. I think we need to first confirm which one is really wanted.

cc @puneetlath

puneetlath commented 1 year ago

Sorry, I'm a bit confused by this issue.

The reproduction steps have a bunch of steps with split bill, before eventually going to the workspace. Is that required to experience the bug? Or does this bug exist with workspace invite no matter what?

As for the behavior, I think we want it to be consistent. So if I'm understanding correctly, on the split bill page you are able to select people with the return key, but in workspace invite, you aren't. Is that right?

dukenv0307 commented 1 year ago

@puneetlath We are using two different components in two pages

  1. In split bill page, we are using OptionSelector component that has the default focusedIndex as 0 so that can work with the key without selecting any option by pressing.

  2. In workspace invite page, we are using SelectionList component that has the default focusedIndex as -1 sp that cannot work with the key if we don't select any option by pressing.

puneetlath commented 1 year ago

I see...interesting. Naive question: why do we have two different components that do such similar things?

dukenv0307 commented 1 year ago

I think we decided to use this component on invite page in this issue

https://github.com/Expensify/App/issues/20354 https://github.com/Expensify/App/issues/11795

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

ntdiary commented 1 year ago

https://github.com/Expensify/App/issues/11795#issuecomment-1655923859

After that we should have no more usages of OptionsSelector, OptionsList and OptionRow, so we proceed to delete those, and related files

I may also be involved in the review of the SelectionList refactor next. The actual result in this OP looks like the expected result of issue #25890. In the future, we will probably also replace all OptionSelector with SelectionList in the future.

So, I feel this issue could be closed, or we could also put it on hold first, until the refactoring is complete?

cc @puneetlath

melvin-bot[bot] commented 1 year ago

@puneetlath, @ntdiary Whoops! This issue is 2 days overdue. Let's get this updated quick!

puneetlath commented 1 year ago

Thanks @ntdiary. I think it's best to close this for now. It seems like what we have is the expected behavior, even if it is inconsistent.

Feel free to comment if you disagree!