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

[$500] Room - First workspace becomes highlighted by default when creating new room for 2nd time #34393

Closed
1 of 6 tasks
lanitochka17 opened this issue Jan 11, 2024 · 43 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 11, 2024

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


Version Number: 1.4.24-4
Reproducible in staging?: Y
Reproducible in production?: Y
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Click on FAB > New Chat > room > workspace
  2. Observe no workspace is highlighted
  3. Select the first workspace
  4. Click outside of the panel to close it
  5. Re do step 1

Expected Result:

No workspace should be highlighted by default

Actual Result:

The first workspace becomes highlighted

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6339358_1705005197988.workspaceRoomHighlighted.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bc4717bf898902a4
  • Upwork Job ID: 1745546481285779456
  • Last Price Increase: 2024-02-01
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 11, 2024
@melvin-bot melvin-bot bot changed the title Room - First workspace becomes highlighted by default when creating new room for 2nd time [$500] Room - First workspace becomes highlighted by default when creating new room for 2nd time Jan 11, 2024
Copy link

melvin-bot bot commented Jan 11, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01bc4717bf898902a4

Copy link

melvin-bot bot commented Jan 11, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 11, 2024
Copy link

melvin-bot bot commented Jan 11, 2024

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

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 11, 2024

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

In selection list, the focusedIndex is set as 0 even tho it should be -1

What is the root cause of that problem?

This happens because when sections change after first render of SelectionList, updateAndScrollToFocusedIndex is called.

It sets the index focused index to 0.

if (sections.length > 0) {
updateAndScrollToFocusedIndex(0);
}

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

Change the dependency array of the useEffect to update only when its length changes [sections?.length]

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jan 11, 2024

Proposal

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

Room - First workspace becomes highlighted by default when creating new room for 2nd time

What is the root cause of that problem?

We don't highlight the first option in the SelectionList because if we don't pass initiallyFocusedOptionKey no option will be focused because the fallback for focusedIndexis set to -1. Due to the updateAndScrollToFocusedIndex, the first option is highlighted sometimes.

if (sections.length > 0) {
updateAndScrollToFocusedIndex(0);
}

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

In my opinion, we should set the fallback for focusedIndex to 0 because in almost every SelectionList components, the first option is always highlighted. I believe it is good for the user experience to indicate that it can also be changed through keyboard navigation.

We can just update this line:

const [focusedIndex, setFocusedIndex] = useState(() => _.findIndex(flattenedSections.allOptions, (option) => option.keyForList === initiallyFocusedOptionKey));

To:

    const [focusedIndex, setFocusedIndex] = useState(() => {
        const initialIndex = _.findIndex(flattenedSections.allOptions, (option) => option.keyForList === initiallyFocusedOptionKey);
        return initialIndex !== -1 ? initialIndex : 0;
    });

Result:

focus_index_selectionList.mp4

Optional:

  1. We can pass initiallyFocusedOptionKey if we don't want to change the fallback for focusedIndex. We will use the first item key in the options form initiallyFocusedOptionKey.
  2. We can introduce a new prop inside BaseSelectionList to highlight the first option only if the prop is true. We will set the fallback property based on that.

@DylanDylann
Copy link
Contributor

I think we should hold for #29080

@sophiepintoraetz
Copy link
Contributor

@aimane-chnaif - do you agree?

@melvin-bot melvin-bot bot removed the Overdue label Jan 14, 2024
@aimane-chnaif
Copy link
Contributor

Agree to hold

@melvin-bot melvin-bot bot added the Overdue label Jan 17, 2024
@aimane-chnaif
Copy link
Contributor

Keep watching progress in #34196

@melvin-bot melvin-bot bot removed the Overdue label Jan 17, 2024
Copy link

melvin-bot bot commented Jan 18, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jan 22, 2024
@aimane-chnaif
Copy link
Contributor

Same

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 22, 2024
@sophiepintoraetz
Copy link
Contributor

No update here - linked holding PR is still in review.

@melvin-bot melvin-bot bot removed the Overdue label Jan 25, 2024
Copy link

melvin-bot bot commented Jan 25, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
@melvin-bot melvin-bot bot removed the Overdue label Feb 28, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 7, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 7, 2024

@aimane-chnaif - Can you please provide an update on this one?

@melvin-bot melvin-bot bot removed the Overdue label Mar 7, 2024
@lschurr lschurr added Daily KSv2 and removed Weekly KSv2 labels Mar 7, 2024
@aimane-chnaif
Copy link
Contributor

Please update proposals based on latest main. BaseSelectionList was refactored much after issue created.

@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 11, 2024

So we're waiting on proposals, is that right @aimane-chnaif?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 11, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 18, 2024

Still waiting on proposals.

@melvin-bot melvin-bot bot removed the Overdue label Mar 18, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 19, 2024

@aimane-chnaif - Should this be open to proposals externally? Is the original bug still happening?

@aimane-chnaif
Copy link
Contributor

aimane-chnaif commented Mar 19, 2024

  1. Observe no workspace is highlighted

This step is failing. First workspace is always highlighted

@aimane-chnaif
Copy link
Contributor

Expected Result:
No workspace should be highlighted by default

Actual Result:
The first workspace becomes highlighted

Should this be inverted?

@lschurr
Copy link
Contributor

lschurr commented Mar 19, 2024

@aimane-chnaif
Copy link
Contributor

I am going to be OOO soon. Please assign new C+.

@aimane-chnaif aimane-chnaif removed their assignment Mar 20, 2024
@dukenv0307
Copy link
Contributor

@lschurr I'm a new C+ so I'm low on issues, I can take over 👍

@lschurr
Copy link
Contributor

lschurr commented Mar 20, 2024

Based on the Slack thread, I think we're going to ask QA to test again. @lanitochka17 can you confirm this bug is still reproducible?

@melvin-bot melvin-bot bot added the Overdue label Mar 25, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 25, 2024

@melvin-bot melvin-bot bot removed the Overdue label Mar 25, 2024
@kavimuru
Copy link

@ishpaul777 bug is not rproducible

bandicam.2024-03-25.18-47-35-654.mp4

@lschurr
Copy link
Contributor

lschurr commented Mar 25, 2024

Closing since we're unable to reproduce.

@lschurr lschurr closed this as completed Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

10 participants