[HOLD for payment 2023-10-23][$2000] Choosing group members change the search input and it's label style

kavimuru 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.Click on the FAB and create New group 2.Start adding the group members and notice the search input and it's label

Expected Result:

Choosing group members shouldn't change the search input and it's label style for better UX.

Actual Result:

Every time we choose a member we change the search input and it's label, because of the search input refocus

Workaround:

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

Platforms:

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

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

Version Number: 1.3.40-1 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 Notes/Photos/Videos: Any additional supporting documentation

Screencast from 13-07-23 05_23_15.webm

https://github.com/Expensify/App/assets/43996225/358c55d1-b9d8-445f-8241-26c76b07f216

Expensify/Expensify Issue URL: Issue reported by: @Ahmed-Abdella Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689216766939829

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01f1e32514bd8c7aa9
Upwork Job ID: 1681446377913671680
Last Price Increase: 2023-08-07
Automatic offers:

situchan | Reviewer | 27097976
s-alves10 | Contributor | 27097978
Ahmed-Abdella | Reporter | 27097981

situchan commented 1 year ago

@s-alves10's proposal looks good to me. I don't see any other better solution to prevent blur on focused search input.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 1 year ago

@tjferriss, @michaelhaxhiu, @MonilBhavsar, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

MonilBhavsar commented 1 year ago

Reviewing now

MonilBhavsar commented 1 year ago

Proposal looks good, but I feel it is not necessary or required to keep focus on search text input, while options are being selected.

looks like we have that in Workspace > Invite members. We don't keep focus on input while selecting workspace invitees. We can do similar here, I think cc @tjferriss @michaelhaxhiu

This looks better and ideal to me

https://github.com/Expensify/App/assets/32012005/0461214c-6501-41f9-9bf1-b08a5d7808e5

situchan commented 1 year ago

@MonilBhavsar Invite page (uses SelectionList) is different than search/new chat/new group pages (uses OptionsList). There was full discussion search and option focus behavior here - https://expensify.slack.com/archives/C01GTK53T8Q/p1692122623311399?thread_ts=1689954073.865489&cid=C01GTK53T8Q. And also updated test rail - https://github.com/Expensify/Expensify/issues/311312

Talha345 commented 1 year ago

Proposal looks good, but I feel it is not necessary or required to keep focus on search text input, while options are being selected.

@MonilBhavsar Yes that is what I suggested too because it feels kinda hacky to me to persist the focus even though the user has moved to another element and we do not know if the user will come back again to the search field or not.I have explained this in detail in my proposal https://github.com/Expensify/App/issues/22812#issuecomment-1658437718.

@situchan If it is decided to move forward with what @MonilBhavsar and I suggested, you can have a look at my proposal using which we can achieve this with minimal changes.

michaelhaxhiu commented 1 year ago

Taking this back over. @MonilBhavsar do you agree with @situchan's comment (above) about how the SelectionList =/= OptionsList, and thus the behavior may not need to be equivalent. The next step lies with you to unblock us :)

MonilBhavsar commented 1 year ago

Commented in the thread. Looks like all agree with keeping dual focus. I think then we should do it similar for all elements across the App https://expensify.slack.com/archives/C01GTK53T8Q/p1692122623311399?thread_ts=1689954073.865489&cid=C01GTK53T8Q

Talha345 commented 1 year ago

Commented in the thread. Looks like all agree with keeping dual focus. I think then we should do it similar for all elements across the App https://expensify.slack.com/archives/C01GTK53T8Q/p1692122623311399?thread_ts=1689954073.865489&cid=C01GTK53T8Q

@MonilBhavsar Apart from this,everywhere we have the standard behavior.I think we should re-think this and instead of changing the behavior at all other places, we should make the behavior in this issue consistent with the entire app and not vice versa.Just my 2 cents.

situchan commented 1 year ago

@Talha345 There are only 2 components which have both search input and selectable list: OptionsList, SelectionList. They represent the entire app.

MonilBhavsar commented 1 year ago

Agree, @Talha345 feel free to share your thoughts in the slack thread

Talha345 commented 1 year ago

@Talha345 There are only 2 components which have both search input and selectable list: OptionsList, SelectionList. They represent the entire app.

@situchan Yes you are right! I was simply talking in terms of behavior without going into the technical terminology to make it consistent across the app.Having focus on multiple elements at once doesn't make sense to me logically but we can proceed with whatever the team decides.

s-alves10 commented 1 year ago

I think keeping focus in search input is a good choice from the view of UX

MonilBhavsar commented 1 year ago

Right, but looks like we want to do something new from industry standard, and allow users to keep typing while also navigate through the list options.

kameshwarnayak commented 1 year ago

Proposal

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

Choosing group members change the search input and it's label style

What is the root cause of that problem?

The root cause of the issue is that when the user clicks on the list item, the focus moves out of the TextInput which is the default behaviour. We focus back on the TextInput in the line below. The time-lag between blur and focus causes the flicker.

https://github.com/Expensify/App/blob/57b76067cd3d7b59e84c4a387d11ad33e6a97d05/src/components/OptionsSelector/BaseOptionsSelector.js#L302

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

TextInput component already has forceActiveLabel prop that allows us to keep the label active. We can use that to prevent the label flickering.
We can define a new state variable keepActive in BaseOptionsSelector.js and pass it to TextInput component.
Set keepActive to true when TextInput is focused
keepActive is set to false when users click outside the TextInput or the Checklist
In BaseTextInput.js apply styles.borderColorFocus if keepActive is true

Idea is to use the existing options in the application to achieve the result with minimal change.

The following code changes can be made to achieve this.

BaseOptionsSelector.js

     /**
     * @param {Boolean} status
     */
    updateKeepActive(status) {
        this.setState({keepActive: status});
    }
    ...
    <TextInput
        keepActive={this.state.keepActive}
        forceActiveLabel={this.state.keepActive}
        onFocus={() => this.updateKeepActive(true)}
        ...
    />
    ...
    <View
        style={[styles.flexGrow1, styles.flexShrink1, styles.flexBasisAuto]}
        onClick={() =>this.updateKeepActive(false)}
    >

BaseTextInput.js

    const textInputContainerStyles = StyleSheet.flatten([
        ...
        (props.keepActive || (!props.hideFocusedState && isFocused)) && styles.borderColorFocus,
        ...
    ]);

Result :

https://github.com/Expensify/App/assets/2232432/62ea888e-582e-419d-8e36-17cc37d93092

What alternative solutions did you explore? (Optional)

None

situchan commented 1 year ago

@kameshwarnayak thanks for your proposal. Label seems to be in fixed position but input cursor still flickers. I don't think your solution is better and straightforward than preventDefault approach.

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu, @MonilBhavsar, @situchan Eep! 4 days overdue now. Issues have feelings too...

MonilBhavsar commented 1 year ago

Still waiting for a correct proposal to generalize things as we discussed in slack.

situchan commented 1 year ago

Still waiting for a correct proposal to generalize things as we discussed in slack.

@MonilBhavsar can you please share the expected behavior finally landed on?

s-alves10 commented 1 year ago

Still waiting for a correct proposal to generalize things as we discussed in slack.

What is the expected behavior? If something was changed, please share here

MonilBhavsar commented 1 year ago

It is in the thread that was linked here https://github.com/Expensify/App/issues/22812#issuecomment-1702566251

We should fix it in more wider way and not just for this group members list. Also, we should explore having a generic component except two different components - OptionsList and SelectionsList

Please see this comment from tgolen in the thread. I agree with this

I'd love for both pages to have consistent UX and I think we should take the time right now to make them such. Is the proposal to make a generic component that can be used on both pages?

And we had couple of thumbs up on this -

Looks like, we agree with doing something different and focussing on both inputs at the same time, then I agree with the point Tim brought above to use similar component or style and have consistency across all elements in the App. If we agree having focus on two elements, then I think we should do it now for all usages inside the App.

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu, @MonilBhavsar, @situchan Eep! 4 days overdue now. Issues have feelings too...

MonilBhavsar commented 1 year ago

Waiting for proposal here

jscardona12 commented 1 year ago

Proposal

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

Choosing group members change the search input and it's label style

What is the root cause of that problem?

Note

The UI is diffrent now than when the error was reported. Now we have same menu for startign a chat or a group.

Also now in Main the search bar is not being refocus.

How its working

https://github.com/Expensify/App/assets/16781411/2be83dc2-8430-4345-a86b-268ad84e913f

When user select members click on add to group or unselect the selected member focus is lost on search bar.

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

In this lines setUp onMouseDown={(e)=>{e.preventDefault()}} to prevent onfocus on the search bar.

https://github.com/Expensify/App/blob/9533031c7795567392cca0b295212b342c9acdfe/src/components/OptionRow.js#L279-L284 and in https://github.com/Expensify/App/blob/9533031c7795567392cca0b295212b342c9acdfe/src/components/OptionRow.js#L286-L291

Solution Working

https://github.com/Expensify/App/assets/16781411/6243b205-eb37-4377-81f8-4848aa2e7670

Refactor to use OptionsList instead of SelectionsList

Right now the Selections List is being used in the following components

We need to change the SelectionsList for the OptionsList in this components. It will require de development of the select All in the Options List. and to test thorughly the features.

@MonilBhavsar currently the UX of both components are diffrent are we goign with the UX of OptionsList correct?

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu, @MonilBhavsar, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu, @MonilBhavsar, @situchan Still overdue 6 days?! Let's take care of this!

MonilBhavsar commented 1 year ago

We recently changed the workflow around creating groups and this issue is not visible any more. The options list in Creating group and splitting money and selection in workspace member settings loses focus when we try to select the options.

If we want to pursue the idea of keeping the focus on search while selecting the option, that could be a feature request I think. @michaelhaxhiu should we consider working on this issue as a feature request, or close it and open new issue, as this bug is not reproducible anymore.

situchan commented 1 year ago

The pages have been refactored but we can still apply solution to these badges:

https://github.com/Expensify/App/assets/108292595/950757e9-52f0-4b08-85cf-26c932a74263

situchan commented 1 year ago

And that's what native app already does:

https://github.com/Expensify/App/assets/108292595/f3a6c2d1-6b33-4f8f-9ec1-a5932daa722c

So good to fix for consistency

s-alves10 commented 1 year ago

Proposal

Updated

MonilBhavsar commented 1 year ago

@situchan Sorry, I didn't get The issue reported originally is not reproducible, is it?

https://github.com/Expensify/App/assets/32012005/5cf27831-44c2-4ef5-a316-7d39e83c5cad

situchan commented 1 year ago

The issue reported originally is not reproducible, is it?

Correct because UX changed and selecting row navigates to another page always.

But code still remaining

https://github.com/Expensify/App/blob/41ddd7428340fd8d347e8160980809e76a93899b/src/pages/NewChatPage.js#L201

jscardona12 commented 1 year ago

@MonilBhavsar it is reproducible, as i show on the videos of my [Proposal] (https://github.com/Expensify/App/issues/22812#issuecomment-1727743878)

Any thoughts on the proposal?

michaelhaxhiu commented 1 year ago

I'm confused myself. Does the bug still exist and need fixing? Or are we saying the bug is fixed from UX perspective and we just need to clean up the code now?

cc @MonilBhavsar

situchan commented 1 year ago

This is not reproducible because of recent regression. Context: https://expensify.slack.com/archives/C01GTK53T8Q/p1695863206217249?thread_ts=1689954073.865489&cid=C01GTK53T8Q Seems like we're going to fix as bug

michaelhaxhiu commented 1 year ago

@situchan nice! thanks for the investigative work. Let's proceed forward provided @MonilBhavsar is in alignment :)

melvin-bot[bot] commented 1 year ago

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

michaelhaxhiu commented 1 year ago

I'm re-assigning this to another BZ as part of my preparation for Sabbatical (starting Friday).

Next steps:

Sherpa this forward with @situchan and @MonilBhavsar
After that regression is fixed, this bug will return to exist and so we may need to wait a few days till that it shows itself again.

MonilBhavsar commented 1 year ago

Looks like the regression is because of https://github.com/Expensify/App/pull/25564/files

melvin-bot[bot] commented 12 months ago

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

sonialiap commented 12 months ago

@MonilBhavsar am I reading this correctly that the regression has not been corrected? If it hasn't, are we solving it in this issue or do we need to locate/open a separate issue for the regression before fixing this bug?

situchan commented 12 months ago

There's no GH created and no one is working on it to fix that "regression". I think we can fix here.

Ahmed-Abdella commented 12 months ago

@sonialiap @situchan This bug can now be reproduced again. I believe the regression responsible for its disappearance has been fixed.

Screencast from 04-10-23 08:43:39.webm

situchan commented 12 months ago

@Ahmed-Abdella Ah, good catch! I just confirmed that PR which fixes https://github.com/Expensify/App/issues/28070 was merged 2 days ago, which made this bug reproducible again.

situchan commented 12 months ago

@s-alves10 can you update your proposal based on latest codebase?

s-alves10 commented 12 months ago

Proposal

Updated

situchan commented 12 months ago

@s-alves10's updated proposal looks good. cc: @MonilBhavsar

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 12 months ago

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

MonilBhavsar commented 11 months ago

@s-alves10 @situchan this does not persist focus on Invite members page that uses selectionList, right?

Expensify / App