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

Add Workspace Members Search Feature #15123

Merged

Conversation

PankajAS
Copy link
Contributor

@PankajAS PankajAS commented Feb 14, 2023

Details

Fixed Issues

$ #12339
PROPOSAL: #12339(comment)

Tests

  1. Navigate to Settings.
  2. Click on Workspaces.
  3. Click on any Workspace and Manage members.
  4. Search member by name or email address.
  • [] Verify that no errors appear in the JS console

Offline tests

  1. Navigate to Settings.
  2. Click on Workspaces.
  3. Click on any Workspace and Manage members.
  4. Search member by name or email address.

QA Steps

  1. Navigate to Settings.
  2. Click on Workspaces.
  3. Click on any Workspace and Manage members.
  4. Search member by name or email address.
  • [] Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (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
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • 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 followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • 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
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Untitled.3.mp4
Mobile Web - Chrome
vidma_recorder_10022023_214530.mp4
Mobile Web - Safari
Untitled.4.mp4
Desktop
Untitled.6.mp4
iOS
Simulator.Screen.Recording.-.iPhone.13.-.2023-02-10.at.18.50.57.mp4
Android
Screen.Recording.2023-02-10.at.8.00.59.PM.mov

@PankajAS PankajAS requested a review from a team as a code owner February 14, 2023 09:38
@melvin-bot melvin-bot bot requested review from 0xmiros and marcaaron and removed request for a team February 14, 2023 09:38
@MelvinBot
Copy link

@marcaaron @0xmiroslav 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]

@PankajAS
Copy link
Contributor Author

PankajAS commented Feb 14, 2023

@0xmiroslav i have created new PR please check, its updated with main branch

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

src/pages/workspace/WorkspaceMembersPage.js Outdated Show resolved Hide resolved
src/pages/workspace/WorkspaceMembersPage.js Outdated Show resolved Hide resolved
src/pages/workspace/WorkspaceMembersPage.js Outdated Show resolved Hide resolved
src/pages/workspace/WorkspaceMembersPage.js Outdated Show resolved Hide resolved
src/pages/workspace/WorkspaceMembersPage.js Outdated Show resolved Hide resolved
@PankajAS
Copy link
Contributor Author

@0xmiroslav I have changed according to these feedbacks:

  1. Added common isKeywordMatch function
  2. Add more fields in filter condition: firstName, lastName
  3. Combine all of these into one if statement
  4. Use common function like clear()

@0xmiros
Copy link
Contributor

0xmiros commented Feb 14, 2023

CLA Assistant Lite bot: Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

You can retrigger this bot by commenting recheck in this Pull Request

I think you need to do this again since old PR is closed

@PankajAS
Copy link
Contributor Author

@0xmiroslav committed these feedbacks

  1. Put this.state.searchValue.trim().toLowerCase() this out of loop
  2. Space formate

@0xmiros
Copy link
Contributor

0xmiros commented Feb 14, 2023

no result
result

@shawnborton please check design (both with result and no result)

@shawnborton
Copy link
Contributor

This looks pretty good to me. I wonder if we can tighten up some of the margins though to take advantage of the space better? Basically remove some pixels from the places I highlighted below:

image

@0xmiros
Copy link
Contributor

0xmiros commented Feb 14, 2023

@PankajAS let's fix @shawnborton's feedback

@PankajAS
Copy link
Contributor Author

PankajAS commented Feb 14, 2023

@0xmiroslav @shawnborton is this looks ok now?
Screenshot 2023-02-14 at 6 01 57 PM

@0xmiros
Copy link
Contributor

0xmiros commented Feb 14, 2023

Looks good to me if you exactly reduced 8px, 4px as @shawnborton stated

@shawnborton
Copy link
Contributor

Yeah, agree - just want to agree that you followed the guidance I provided before signing off. It looks like maybe more than 4 was removed from above the search input?

@PankajAS
Copy link
Contributor Author

Yeah, agree - just want to agree that you followed the guidance I provided before signing off. It looks like maybe more than 4 was removed from above the search input?

I have just remove 4px @shawnborton as i set styles.pt1 in place of styles.pt5

@shawnborton
Copy link
Contributor

shawnborton commented Feb 14, 2023

Ah, well the difference between pt1 (4px) and pt5 (20px) is actually 16. So really you would want to use pt4 instead of pt5.

@shawnborton shawnborton self-requested a review February 14, 2023 13:00
@PankajAS
Copy link
Contributor Author

Ah, well the difference between pt1 (4px) and pt5 (20px) is actually 16. So really you would want to use pt4 instead of pt5.

Ohk @shawnborton

@0xmiros
Copy link
Contributor

0xmiros commented Feb 14, 2023

@PankajAS since you're new, maybe you are not familiar with our style guidelines.
Styles are defined in styles.js and 1 difference is 4px.
i.e.

    pt2: {
        paddingTop: 8,
    },

    pt3: {
        paddingTop: 12,
    },

    pt4: {
        paddingTop: 16,
    },

    pt5: {
        paddingTop: 20,
    },

You can use spacing already defined in /styles/utilities/spacing.js

@PankajAS
Copy link
Contributor Author

@shawnborton looks ok now?

Screenshot 2023-02-14 at 6 32 44 PM

@PankajAS
Copy link
Contributor Author

PankajAS commented Feb 14, 2023

@PankajAS since you're new, maybe you are not familiar with our style guidelines. Styles are defined in styles.js and 1 difference is 4px. i.e.

    pt2: {
        paddingTop: 8,
    },

    pt3: {
        paddingTop: 12,
    },

    pt4: {
        paddingTop: 16,
    },

    pt5: {
        paddingTop: 20,
    },

You can use spacing already defined in /styles/utilities/spacing.js

yes just checked and i'm using these styles @0xmiroslav

@shawnborton
Copy link
Contributor

@PankajAS and to be clear, you only removed 4 from below the search input as well?

@PankajAS
Copy link
Contributor Author

PankajAS commented Feb 14, 2023

@PankajAS and to be clear, you only removed 4 from below the search input as well?

yes @shawnborton i remove 4px from below and top of search input

@shawnborton
Copy link
Contributor

Cool, thank you. That works for me then!

@PankajAS
Copy link
Contributor Author

@mountiny I have signed CLA in dummy PR

@PankajAS
Copy link
Contributor Author

its done @mountiny

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a saga, thanks everyone
image

@mountiny
Copy link
Contributor

@PankajAS Seems like the job 3 is not succeeding, I have retried it, its not related but maybe if it fails again, syncing main branch with this might be helpful

@PankajAS
Copy link
Contributor Author

@PankajAS Seems like the job 3 is not succeeding, I have retried it, its not related but maybe if it fails again, syncing main branch with this might be helpful

Ok @mountiny

@mountiny
Copy link
Contributor

@PankajAS can you merge in main please?

@PankajAS
Copy link
Contributor Author

PankajAS commented Feb 22, 2023

@PankajAS can you merge in main please?

do i need to update PR branch with main branch? @mountiny

@PankajAS
Copy link
Contributor Author

this branch is updated @mountiny with main branch

@mountiny
Copy link
Contributor

@PankajAS I think you need to pull the latest main from Expensify/App and sync it with your fork and merge that in. You commit if 2 days old and there were PRs merged into main since then

@PankajAS
Copy link
Contributor Author

@PankajAS I think you need to pull the latest main from Expensify/App and sync it with your fork and merge that in. You commit if 2 days old and there were PRs merged into main since then

Ok let me sync with main from Expensify/App

1. Added common isKeywordMatch function
2. Add more fields in filter condition: firstName, lastName
3. Combine all of these into one if statement
4. Use common function like clear()

1. Put this.state.searchValue.trim().toLowerCase() this out of loop
2. Space formate

UI Adjustments

Match "No results found" text with new chat text

Fix lint errors

1. Change to "Clear search input value and selected members"
2. change selectedEmployees to selectedMembers
3. Change to "Check if value matches with keyword"

1. Fix lint changes
2. Add policyMemberList check for "no results found" text

Fix Space lint

1. Add JSDoc for onSearch function
2. Remove commnet for Clear function
3. Change Clear method name to clearMembersAndSearchValue
4. Revert selectedMembers name with selectedEmployees

1. Remove updateSearchValue Comment
2. Update OnSearch name to updateSearchValue
3. Inline conditions for Filter list data

Remove clearMembersAndSearchValue method

1. Remove unnecessary safety condition
2.Remove unnecessary comment for this function
3. split into multiple lines so fix lint

Add singinkey

Revert "Add singinkey"

This reverts commit d8b3e83fda9fbb07a3bb865cc7fa5b255778787d.

remove extra line

Revert "remove extra line"

This reverts commit 022e15693390fae1a8e1eeb4c6c483c21e61cc2f.

Update src/pages/workspace/WorkspaceMembersPage.js

Co-authored-by: Miroslav Stevanovic <97473779+0xmiroslav@users.noreply.github.com>

Fix lint errors

1. Change to "Clear search input value and selected members"
2. change selectedEmployees to selectedMembers
3. Change to "Check if value matches with keyword"

1. Fix lint changes
2. Add policyMemberList check for "no results found" text

Fix Space lint

1. Add JSDoc for onSearch function
2. Remove commnet for Clear function
3. Change Clear method name to clearMembersAndSearchValue
4. Revert selectedMembers name with selectedEmployees

Remove clearMembersAndSearchValue method

1. Remove unnecessary safety condition
2.Remove unnecessary comment for this function
3. split into multiple lines so fix lint

Add singinkey

Revert "Add singinkey"

This reverts commit d8b3e83fda9fbb07a3bb865cc7fa5b255778787d.

remove extra line

Revert "remove extra line"

This reverts commit 022e15693390fae1a8e1eeb4c6c483c21e61cc2f.
@PankajAS
Copy link
Contributor Author

Done @mountiny

@PankajAS
Copy link
Contributor Author

now we are all set to merge PR? @mountiny @0xmiroslav

@mountiny
Copy link
Contributor

@PankajAS have to wait for the actions to succeed. Seems like there is something odd going on with the job3

@PankajAS
Copy link
Contributor Author

Ok @mountiny

@mountiny
Copy link
Contributor

Looking good

@PankajAS
Copy link
Contributor Author

Great @mountiny

@mountiny mountiny merged commit ff218f0 into Expensify:main Feb 22, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 725.112 ms → 746.870 ms (+21.758 ms, +3.0%)
App start runJsBundle 199.065 ms → 208.344 ms (+9.279 ms, +4.7%)
Open Search Page TTI 621.384 ms → 625.869 ms (+4.485 ms, +0.7%)
App start regularAppStart 0.015 ms → 0.015 ms (+0.000 ms, +0.7%)
App start nativeLaunch 20.034 ms → 19.607 ms (-0.427 ms, -2.1%)
Show details
Name Duration
App start TTI Baseline
Mean: 725.112 ms
Stdev: 30.460 ms (4.2%)
Runs: 649.7012199983001 678.6512060016394 682.2082169987261 694.2664370015264 694.5936469994485 698.657283000648 703.1484780013561 705.1559920012951 710.3216650001705 711.9329599998891 715.5712799988687 720.2964930012822 723.1315260007977 723.2365959994495 726.4176979996264 727.8158890008926 727.8574559986591 730.6039739996195 731.6379529982805 734.0204639993608 736.220205001533 736.8379519991577 741.5839500017464 743.9118129983544 751.8321620002389 757.7641360014677 758.2968560010195 763.2112980000675 764.172008998692 810.309089999646

Current
Mean: 746.870 ms
Stdev: 25.261 ms (3.4%)
Runs: 709.1260860003531 710.4664850011468 714.9349469989538 715.2159750014544 719.5283619984984 721.092402998358 723.1689190007746 730.3369229994714 732.4177489988506 733.0570789985359 733.7682740017772 735.0476859994233 735.7343589998782 739.0696029998362 739.9443209990859 740.6873530000448 742.4444160014391 744.8895029984415 747.3921479992568 747.7197390012443 750.1894319988787 752.8928840011358 753.9241020008922 762.8978900015354 767.2333379983902 771.1556550003588 775.5725560002029 775.715569999069 778.3437279984355 789.9991989992559 791.941454000771 813.931683998555
App start runJsBundle Baseline
Mean: 199.065 ms
Stdev: 17.620 ms (8.9%)
Runs: 171 177 180 180 182 183 186 187 188 188 188 190 191 194 194 195 196 197 198 201 201 204 207 209 214 217 220 225 229 231 248

Current
Mean: 208.344 ms
Stdev: 23.588 ms (11.3%)
Runs: 172 175 177 181 182 186 187 190 191 192 193 195 198 203 203 205 207 209 211 214 214 216 216 220 224 227 229 235 242 255 258 260
Open Search Page TTI Baseline
Mean: 621.384 ms
Stdev: 19.607 ms (3.2%)
Runs: 588.1806640028954 589.6177570000291 590.8625079989433 591.4526369981468 596.3271079994738 597.8956709988415 600.0006510019302 603.2921149991453 609.9108070023358 613.0231940001249 613.7534190006554 615.8803710006177 616.8697919994593 616.946532998234 621.1888030022383 622.4178470000625 623.2230639979243 627.4318850003183 627.4927169978619 629.6363120004535 630.7617189995944 632.1824950017035 633.2994390018284 633.6961669996381 634.6375739984214 635.7795010022819 636.3204759992659 637.3554280027747 643.5999759994447 646.3810639977455 656.3258059993386 668.5389409996569

Current
Mean: 625.869 ms
Stdev: 17.729 ms (2.8%)
Runs: 586.4264330007136 595.4175620004535 597.7784020006657 600.8223470002413 600.9484050013125 609.500489000231 611.0585529990494 612.2175709977746 613.4984939992428 615.6113689988852 617.7219650000334 619.6688640005887 619.8304040022194 621.0907389968634 625.0320230014622 625.2679450027645 626.3769939988852 627.3809010013938 627.5080980025232 631.6430260017514 632.2987469993532 633.4239090010524 633.4496669992805 640.8269449993968 641.1496990025043 645.5858150012791 647.2386070005596 647.7475989982486 648.260009996593 648.4552000015974 649.718384001404 650.2478029988706 650.468586999923
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (5.4%)
Runs: 0.013305000960826874 0.013549000024795532 0.013753999024629593 0.013916000723838806 0.01395600289106369 0.014038000255823135 0.014201000332832336 0.014242000877857208 0.014282997697591782 0.014322999864816666 0.014364000409841537 0.014403998851776123 0.014444999396800995 0.014485999941825867 0.014566998928785324 0.01460699737071991 0.014647997915744781 0.014770999550819397 0.014811001718044281 0.015054997056722641 0.015096001327037811 0.015096001327037811 0.015136998146772385 0.015178002417087555 0.015380002558231354 0.015380997210741043 0.015585001558065414 0.016275998204946518 0.016479000449180603 0.016682997345924377

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.7%)
Runs: 0.013346001505851746 0.013427000492811203 0.01359100267291069 0.01371300220489502 0.013794001191854477 0.013835001736879349 0.014079000800848007 0.01411999762058258 0.014120001345872879 0.01424100250005722 0.014282003045082092 0.014364000409841537 0.014689002186059952 0.014730002731084824 0.014851998537778854 0.014892000705003738 0.014932997524738312 0.015014998614788055 0.015095997601747513 0.015136998146772385 0.015217997133731842 0.015258997678756714 0.015381000936031342 0.015381000936031342 0.015461999922990799 0.01550300046801567 0.015585001558065414 0.015828002244234085 0.015869002789258957 0.01603199914097786 0.016194000840187073 0.01672399789094925
App start nativeLaunch Baseline
Mean: 20.034 ms
Stdev: 2.141 ms (10.7%)
Runs: 17 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 20 20 21 21 21 21 22 22 23 26 26

Current
Mean: 19.607 ms
Stdev: 1.448 ms (7.4%)
Runs: 17 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 22 22 23 23

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.76-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

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

1 similar comment
@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

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

@sobitneupane
Copy link
Contributor

Dismissing keyboard on scroll on native devices(Android and IOS) was not considered in this PR. The issue was later reported and solved by this PR

Steps to reproduce:

  • Open manage members page of any workspace on Android.
  • Tap on search and scroll through members
  • The keyboard is expected to be dismissed (similar to search page, invite member page)

@0xmiros
Copy link
Contributor

0xmiros commented Mar 28, 2023

Dismissing keyboard on scroll on native devices(Android and IOS) was not considered in this PR.

It was already considered as Issue 2 in #15123 (comment)
And we agreed to do nothing (or out of scope) for this PR
Screenshot 2023-03-28 at 2 43 39 PM

@sobitneupane
Copy link
Contributor

sobitneupane commented Apr 11, 2023

There was some consistency issue not considered in this PR. Search value was not being cleared while moving between the pages.

Steps to reproduce:

  • Write something in Search Input in Manage Members page
  • Click Invite members then Go back
  • Notice that the search value is still present

@0xmiros
Copy link
Contributor

0xmiros commented Apr 11, 2023

There was some consistency issue not considered in this PR. Search value was not being cleared while moving between the pages.

Steps to reproduce:

  • Write something in Search Input in Manage Members page
  • Click Invite members then Go back
  • Notice that the search value is still present

We already discussed that concern here but decided won't fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants