App icon indicating copy to clipboard operation
App copied to clipboard

[HOLD for payment 2024-12-09] [$125] Make sure the first result in most recents is highlighted when user uses CMD+K

Open JmillsExpensify opened this issue 1 year ago β€’ 33 comments

From the design doc.

Recent chats is the same as our existing CMD+K command, which means:

  • The router can be opened and focused via the same keyboard shortcut
  • When CMD+K is pressed and no search has been entered, the top-most result in Recent chats is highlighted. Hitting the return key closes the router and navigates a member to the corresponding report

CleanShot 2024-11-01 at 19 44 45@2x

cc @luacmartins

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021852421610290735954
  • Upwork Job ID: 1852421610290735954
  • Last Price Increase: 2024-11-01
  • Automatic offers:
    • nyomanjyotisa | Contributor | 104805899
Issue OwnerCurrent Issue Owner: @JmillsExpensify

JmillsExpensify avatar Nov 01 '24 18:11 JmillsExpensify

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

melvin-bot[bot] avatar Nov 01 '24 18:11 melvin-bot[bot]

Current assignee @JmillsExpensify is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] avatar Nov 01 '24 18:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 01 '24 18:11 melvin-bot[bot]

Upwork job price has been updated to $125

melvin-bot[bot] avatar Nov 01 '24 18:11 melvin-bot[bot]

Proposal

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

Make sure the first result in most recents is highlighted when user uses CMD+K

What is the root cause of that problem?

Improvement

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

We should save the first chat index here https://github.com/Expensify/App/blob/bfce2771d22cbecceee6cf3c064fe3f0627eb5d8/src/components/Search/SearchRouter/SearchRouterList.tsx#L176

    const initiallyFocusedOptionKey = styledRecentReports.at(0)?.keyForList;

we will pass it here https://github.com/Expensify/App/blob/bfce2771d22cbecceee6cf3c064fe3f0627eb5d8/src/components/Search/SearchRouter/SearchRouterList.tsx#L240 Additionally: From the OP I can see that the list is not suppose to scrolled to the focused item on initial render but we have a code here that scroll to the focused index https://github.com/Expensify/App/blob/bfce2771d22cbecceee6cf3c064fe3f0627eb5d8/src/components/SelectionList/BaseSelectionList.tsx#L514 we will disable this code by passing a new param shouldNotScrollToFocusedIndex

What alternative solutions did you explore? (Optional)

FitseTLT avatar Nov 01 '24 20:11 FitseTLT

Proposal

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

Make sure the first result in most recents is highlighted when user uses CMD+K

What is the root cause of that problem?

I don't know if the above proposal states the same issue or not, but for me; the issue is the lack of adding the initiallyFocusedOptionKey prop to the <SelectionList/> component.

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

  • Adding initiallyFocusedOptionKey={recentReports && recentReports.length > 0 ? recentReports.at(0)?.keyForList : ''} prop after the onArrowFocus prop will make sure that the first chat will be focused in recent chats if there are any. https://github.com/Expensify/App/blob/bef062b4caa7f665159dc107911e708031e648c4/src/components/Search/SearchRouter/SearchRouterList.tsx#L240

What alternative solutions did you explore? (Optional)

HusseinSamy avatar Nov 02 '24 21:11 HusseinSamy

πŸ“£ @HusseinSamy! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

melvin-bot[bot] avatar Nov 02 '24 21:11 melvin-bot[bot]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01dd32e60eb083d581

HusseinSamy avatar Nov 02 '24 21:11 HusseinSamy

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] avatar Nov 02 '24 21:11 melvin-bot[bot]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~0139a3178534697447

deking254 avatar Nov 03 '24 17:11 deking254

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] avatar Nov 03 '24 17:11 melvin-bot[bot]

Proposal

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

Make sure the first result in most recents is highlighted when user uses CMD+K

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

The lack of an initiallyFocusedOptionKey to set the default focused item within the <SelectionList> component causes the list to open without any item pre-highlighted.

To ensure the first chat in the recent chats list is automatically highlighted when the search bar opens with CMD+K, we will:

  1. Add a variable, initiallyFocusedOptionKey, set to the key of the first recent chat.
  2. Pass this variable as a prop to the <SelectionList> component, enabling automatic highlighting.

https://github.com/Expensify/App/blob/bef062b4caa7f665159dc107911e708031e648c4/src/components/Search/SearchRouter/SearchRouterList.tsx#L176

const initiallyFocusedOptionKey = styledRecentReports.at(0)?.keyForList;

and we will pass initiallyFocusedOptionKey to <SelectionList>

https://github.com/Expensify/App/blob/bef062b4caa7f665159dc107911e708031e648c4/src/components/Search/SearchRouter/SearchRouterList.tsx#L240

initiallyFocusedOptionKey={initiallyFocusedOptionKey}

What alternative solutions did you explore? (Optional)

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~017a4e9874f15ccbd9

samranahm avatar Nov 03 '24 19:11 samranahm

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] avatar Nov 03 '24 19:11 melvin-bot[bot]

@samranahm Your proposal will be dismissed because you did not follow the proposal template.

github-actions[bot] avatar Nov 03 '24 19:11 github-actions[bot]

Edited by proposal-police: This proposal was edited at 2024-11-04 10:01:40 UTC.

Proposal

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

Make sure the first result in most recents is highlighted when user uses CMD+K

What is the root cause of that problem?

Changes request

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

All proposals above not working if we reload the page and directly press CMD+K, also not working if the user remove the search term. So I propose the following:

Create a new function called setInitialFocus here, and calculated the initialFocusIndex base on sortedRecentSearches and contextualReportData

    const setInitialFocus = useCallback(() => {
        const initialFocusIndex = sortedRecentSearches?.slice(0, 5).length + (contextualReportData ? 1 : 0);
        listRef.current?.setFocusedIndex(initialFocusIndex);
        listRef.current?.scrollToIndex(0, false);
    }, [sortedRecentSearches, contextualReportData]);

Call it in useEffect

    useEffect(() => {
        setInitialFocus();
    }, [sortedRecentSearches]);

And on onSearchChange, if newUserQuery false https://github.com/Expensify/App/blob/0647baf7b70967a707df6eae6cb4c3f5a260a8f5/src/components/Search/SearchRouter/SearchRouter.tsx#L307-L309

            } else {
                setInitialFocus();
            }

POC

https://github.com/user-attachments/assets/fd61f92f-76b8-4ae0-9200-e7739b62ad83

What alternative solutions did you explore? (Optional)

nyomanjyotisa avatar Nov 04 '24 09:11 nyomanjyotisa

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

melvin-bot[bot] avatar Nov 04 '24 18:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 05 '24 18:11 melvin-bot[bot]

@Pujan92 did you get a chance to look at the proposals above?

luacmartins avatar Nov 05 '24 23:11 luacmartins

Yes, let's please prioritize this one. It's coming up actively with our members.

JmillsExpensify avatar Nov 06 '24 18:11 JmillsExpensify

Sorry for the delay, I will review the proposals today.

Pujan92 avatar Nov 07 '24 11:11 Pujan92

Usually initiallyFocusedOptionKey is a straightforward option which also consistent with other places but it won't work for all scenarios(1. Reload and the context may not be available initially 2. When a search term is cleared) as @nyomanjyotisa mentioned. With that, I think @nyomanjyotisa's proposal of setting the focused index explicitly makes sense.

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

Pujan92 avatar Nov 08 '24 06:11 Pujan92

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] avatar Nov 08 '24 06:11 melvin-bot[bot]

πŸ“£ @nyomanjyotisa πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 πŸ“–

melvin-bot[bot] avatar Nov 08 '24 15:11 melvin-bot[bot]

@JmillsExpensify, @Pujan92, @luacmartins, @nyomanjyotisa Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Nov 11 '24 15:11 melvin-bot[bot]

@JmillsExpensify, @Pujan92, @luacmartins, @nyomanjyotisa Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Nov 13 '24 09:11 melvin-bot[bot]

PR is being worked on. Hopefully up for review this week.

JmillsExpensify avatar Nov 13 '24 13:11 JmillsExpensify

⚠️ Looks like this issue was linked to a Deploy Blocker 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 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[bot] avatar Dec 02 '24 20:12 melvin-bot[bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] avatar Dec 02 '24 21:12 melvin-bot[bot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.69-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

  • https://github.com/Expensify/App/pull/52298

If no regressions arise, payment will be issued on 2024-12-09. :confetti_ball:

For reference, here are some details about the assignees on this issue:

  • @Pujan92 requires payment through NewDot Manual Requests
  • @nyomanjyotisa requires payment automatic offer (Contributor)

melvin-bot[bot] avatar Dec 02 '24 21:12 melvin-bot[bot]

@Pujan92 @JmillsExpensify @Pujan92 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot[bot] avatar Dec 02 '24 21:12 melvin-bot[bot]