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.59k stars 2.93k forks source link

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

Open JmillsExpensify opened 1 month ago

JmillsExpensify commented 1 month ago

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
melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Upwork job price has been updated to $125

FitseTLT commented 1 month ago

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)

HusseinSamy commented 1 month ago

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?

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 month ago

πŸ“£ @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>
HusseinSamy commented 1 month ago

Contributor details Your Expensify account email: hussein.samy02@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01dd32e60eb083d581

melvin-bot[bot] commented 1 month ago

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

deking254 commented 1 month ago

Contributor details Your Expensify account email: dekingsky@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0139a3178534697447

melvin-bot[bot] commented 1 month ago

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

samranahm commented 1 month ago

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: samranahmed03@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~017a4e9874f15ccbd9

melvin-bot[bot] commented 1 month ago

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

github-actions[bot] commented 1 month ago

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

nyomanjyotisa commented 4 weeks ago

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)

melvin-bot[bot] commented 4 weeks ago

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

melvin-bot[bot] commented 4 weeks ago

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

luacmartins commented 3 weeks ago

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

JmillsExpensify commented 3 weeks ago

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

Pujan92 commented 3 weeks ago

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

Pujan92 commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

πŸ“£ @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] commented 3 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

JmillsExpensify commented 2 weeks ago

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

melvin-bot[bot] commented 23 hours ago

⚠️ 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] commented 22 hours ago

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

melvin-bot[bot] commented 22 hours ago

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:

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:

melvin-bot[bot] commented 22 hours ago

@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]