Fix for: Chat - New Group - The search box overlays the selected user #14175

Ollyws · 2023-01-10T12:29:50Z

Details

Fixed Issues

$ #14001
PROPOSAL: #14001 (comment)

Tests

Click on the green + button
Click on the "New group" option
Start typing a search term
Select user
Scroll down until the selected accounts are not visible
Clear the search box
(If search results change) Verify list scrolls to the top and all selected users are visible.

Verify that no errors appear in the JS console

Offline tests

Click on the green + button
Click on the "New group" option
Start typing a search term
Select user
Scroll down until the selected accounts are not visible
Clear the search box
(If search results change) Verify list scrolls to the top and all selected users are visible.

QA Steps

Click on the green + button
Click on the "New group" option
Start typing a search term
Select user
Scroll down until the selected accounts are not visible
Clear the search box
(If search results change) Verify list scrolls to the top and all selected users are visible.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

MacOS_Chrome.mp4

Mobile Web - Chrome

Android_Chrome.mp4

Mobile Web - Safari

iOS_Safari.mp4

Desktop

MacOS_Desktop.mp4

iOS

iOS_Native.mp4

Android

Android_Native.mp4

melvin-bot · 2023-01-10T12:29:57Z

@flodnv @mollfpr One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

mollfpr · 2023-01-10T12:36:25Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

14175.Web.mov

Mobile Web - Chrome

14175.mWeb-Chrome.mov

Mobile Web - Safari

14175.mWeb-Safari.mov

Desktop

14175.Desktop.mov

iOS

14175.iOS.mov

Android

14175.Android.mov

mollfpr · 2023-01-10T13:25:45Z

@Ollyws I think we should add a point where we need to scroll after selecting an account and then clear the search input. The solved issue will be more visible.

Ollyws · 2023-01-10T13:27:53Z

@mollfpr In the testing steps you mean? Could you give me an example? Thanks.

mollfpr · 2023-01-10T13:33:42Z

1. Click on the green + button
2. Click on the "New group" option
3. Start typing a search term
4. Select user
5. Scroll down until the selected accounts is not visible
6. Clear the search box
7. (If search results change) Verify list scrolls to the top and all selected users are visible.

Like this.

Ollyws · 2023-01-10T13:35:49Z

@mollfpr Ah yes I see, testing steps updated.

mollfpr

LGTM 👍

All yours @flodnv

flodnv

Looks great, thanks! 👍

flodnv · 2023-01-10T13:51:59Z

src/components/OptionsSelector/BaseOptionsSelector.js

@@ -108,7 +108,7 @@ class BaseOptionsSelector extends Component {
            focusedIndex: newFocusedIndex,
        }, () => {
            // If we just toggled an option on a multi-selection page, scroll to top


Should we update this comment perhaps?

Ahh yes, we should add more context about the changes.

@Ollyws Can you update the comment there? Thanks!

Good idea, done.

flodnv

👌

github-actions · 2023-01-10T15:13:54Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	658.480 ms → 677.904 ms (+19.424 ms, +2.9%)
App start nativeLaunch	9.250 ms → 19.581 ms (+10.331 ms, +111.7%) 🟡
App start runJsBundle	183.875 ms → 188.188 ms (+4.313 ms, +2.3%)
App start regularAppStart	0.013 ms → 0.020 ms (+0.007 ms, +50.4%) 🟡
Open Search Page TTI	603.510 ms → 599.759 ms (-3.751 ms, -0.6%)

Show details

Name	Duration
App start TTI	Baseline Mean: 658.480 ms Stdev: 31.939 ms (4.9%) Runs: 605.4564179999288 606.3217430000659 608.4134440000635 618.7776109999977 619.0414879999589 628.2682239999995 628.694448000053 630.4242769998964 633.2493809999432 641.6933329999447 643.2926250000019 653.3236730000935 653.6824539999943 655.4730130000971 656.0441340000834 656.3471429999918 657.9054960000794 659.3742949999869 667.7503839998972 674.2070929999463 674.9365099999122 675.8589560000692 679.0071709998883 680.8920430000871 684.065152999945 684.9051220000256 688.3203169999178 694.9702590000816 703.88650100003 707.4616829999723 740.8297099999618 Current Mean: 677.904 ms Stdev: 31.546 ms (4.7%) Runs: 621.2411130000837 632.4146040000487 640.4372549999971 640.7232049999293 642.605964000104 643.584944000002 645.0619469999801 645.2613880001009 646.451996000018 655.8934249999002 655.979493000079 656.8787299999967 660.853134999983 668.1945640000049 671.3651620000601 674.0395959999878 683.5045010000467 688.2386030000634 692.3567860000767 694.5844350000843 694.9751850001048 696.9868079998996 698.6787819999736 700.4323950000107 701.4588510000613 707.3143589999527 711.0333229999524 713.3065909999423 715.2383510000072 722.3380670000333 723.0469339999836 748.44188700011
App start nativeLaunch	Baseline Mean: 9.250 ms Stdev: 0.871 ms (9.4%) Runs: 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 10 10 10 11 11 Current Mean: 19.581 ms Stdev: 1.432 ms (7.3%) Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 22 22 23 23
App start runJsBundle	Baseline Mean: 183.875 ms Stdev: 22.416 ms (12.2%) Runs: 148 153 154 156 158 164 165 166 169 170 171 171 174 176 180 180 181 181 187 187 188 190 197 197 198 206 208 209 213 219 229 239 Current Mean: 188.188 ms Stdev: 20.312 ms (10.8%) Runs: 156 162 164 165 166 167 168 171 171 175 177 179 181 182 183 183 185 188 190 191 192 198 198 199 202 202 210 211 214 223 234 235
App start regularAppStart	Baseline Mean: 0.013 ms Stdev: 0.001 ms (5.6%) Runs: 0.011881999904289842 0.01216600020416081 0.012368999887257814 0.012410000199452043 0.012654999969527125 0.012777000200003386 0.012817999813705683 0.012817999813705683 0.01293900003656745 0.013061000034213066 0.013224000111222267 0.013264999957755208 0.013264999957755208 0.013305999804288149 0.013306000037118793 0.013306000037118793 0.013306000037118793 0.013387999963015318 0.013428000034764409 0.013508999953046441 0.013508999953046441 0.013589999871328473 0.014038000022992492 0.014160000020638108 0.014160000020638108 0.014241999946534634 0.014445000095292926 0.015503000002354383 Current Mean: 0.020 ms Stdev: 0.001 ms (5.4%) Runs: 0.01798499980941415 0.018595000030472875 0.018920999951660633 0.018962000031024218 0.019084000028669834 0.019205000018700957 0.019246000098064542 0.019286999944597483 0.019367999862879515 0.01936800009571016 0.01953100017271936 0.019613000098615885 0.019613000098615885 0.019652999937534332 0.019694000016897917 0.019898000173270702 0.01993800001218915 0.019979000091552734 0.02010100008919835 0.020141999935731292 0.020385999931022525 0.020385999931022525 0.02038600016385317 0.02095600008033216 0.0210359999909997 0.021524999989196658 0.021524999989196658 0.021850999910384417 0.022013999987393618 0.022744999965652823
Open Search Page TTI	Baseline Mean: 603.510 ms Stdev: 21.384 ms (3.5%) Runs: 560.8776449998841 566.9577240000945 570.8043210001197 577.7796229999512 582.241251999978 583.15429700003 583.3835050000343 586.7851970000193 587.5784909999929 589.3029380000662 592.7006019998807 594.4872240000404 600.5723870000802 603.560913000023 603.744752000086 604.0190020001028 604.4265950000845 604.6559250000864 606.7329919999465 608.6543379998766 612.6905110001098 614.0214849999174 615.579875000054 616.0019939998165 617.1903079999611 620.3568110000342 623.1741940001957 625.8023679999169 626.3807380001526 629.8865149999037 639.1125089998823 659.698364000069 Current Mean: 599.759 ms Stdev: 16.856 ms (2.8%) Runs: 564.9704590002075 570.730835000053 571.2663980000652 573.7462570001371 581.6757819999475 582.5906169998925 586.0115970000625 587.5170899999794 588.4452730000485 589.9661060001235 592.1055919998325 594.7384440000169 596.6277270000428 597.0383710002061 597.199137999909 599.9553230002057 600.5727130000014 603.1681309998967 603.5586349999066 605.0354410000145 605.9519859999418 606.384684999939 609.3574629998766 609.5261239998508 611.1518969996832 611.3610030000564 614.1880699999165 615.5273840001319 615.6306559999939 617.9438070000615 620.7425540001132 624.3795569997746 642.9807539999019

OSBotify · 2023-01-10T17:07:24Z

🚀 Deployed to staging by @flodnv in version: 1.2.52-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-01-12T13:38:00Z

🚀 Deployed to production by @Julesssss in version: 1.2.52-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

aimane-chnaif · 2023-10-16T17:26:14Z

src/components/OptionsSelector/BaseOptionsSelector.js

-            // If we just toggled an option on a multi-selection page, scroll to top
-            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length) {
+            // If we just toggled an option on a multi-selection page or cleared the search input, scroll to top
+            if (this.props.selectedOptions.length !== prevProps.selectedOptions.length || this.props.value === '') {


This change caused The scrollbar automatically scrolls to the top when one of contacts update their display name.
As the comment says, this should be called only when search input is cleared.
So we fixed this condition to !!prevProps.value && !this.props.value

Added scroll to top when search input is cleared

e9ac89e

Ollyws requested a review from a team as a code owner January 10, 2023 12:29

melvin-bot bot requested review from flodnv and mollfpr and removed request for a team January 10, 2023 12:29

mollfpr previously approved these changes Jan 10, 2023

View reviewed changes

flodnv reviewed Jan 10, 2023

View reviewed changes

Updated comment for scroll to top

8b47a67

Ollyws dismissed mollfpr’s stale review via 8b47a67 January 10, 2023 13:55

mollfpr approved these changes Jan 10, 2023

View reviewed changes

flodnv approved these changes Jan 10, 2023

View reviewed changes

flodnv merged commit 4ad25a0 into Expensify:main Jan 10, 2023

OSBotify mentioned this pull request Jan 10, 2023

Deploy Checklist: New Expensify 2023-01-10 #14188

Closed

15 tasks

melvin-bot bot mentioned this pull request Jan 12, 2023

[HOLD for payment 2023-01-19] [$1000] Chat - New Group - The search box overlays the selected user #14001

Closed

6 tasks

Ollyws deleted the issue-14001 branch February 14, 2023 12:03

aimane-chnaif reviewed Oct 16, 2023

View reviewed changes

aimane-chnaif mentioned this pull request Oct 16, 2023

[HOLD for payment 2023-10-16] [$500] New chat - The scrollbar automatically scrolls to the top in user contact list. #25947

Closed

6 tasks

Fix for: Chat - New Group - The search box overlays the selected user #14175

Fix for: Chat - New Group - The search box overlays the selected user #14175

Uh oh!

Conversation

Ollyws commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Jan 10, 2023

Uh oh!

mollfpr commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

mollfpr commented Jan 10, 2023

Uh oh!

Ollyws commented Jan 10, 2023

Uh oh!

mollfpr commented Jan 10, 2023

Uh oh!

Ollyws commented Jan 10, 2023

Uh oh!

mollfpr left a comment

Choose a reason for hiding this comment

Uh oh!

flodnv left a comment

Choose a reason for hiding this comment

Uh oh!

flodnv Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

mollfpr Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

Ollyws Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

flodnv left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 10, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

Uh oh!

OSBotify commented Jan 10, 2023

Uh oh!

OSBotify commented Jan 12, 2023

Uh oh!

aimane-chnaif Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ollyws commented Jan 10, 2023 •

edited

Loading

mollfpr commented Jan 10, 2023 •

edited

Loading