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

feat: [M3-7883] - Add search and alphabetical sorting to Switch Account drawer #10515

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented May 23, 2024

Description 📝

This PR brings two improvements to the Switch Account drawer, allowing users with hundreds of child accounts to more easily navigate that list:

  1. Search capabilities on the company name
  2. Alphabetization of the list of company names

We are already incrementally fetching the list using an infinite query, but the existing UI still requires scrolling and visually searching, which can be a burden on the user.

Changes 🔄

  • Sorted the list of child accounts alphabetically.
  • Added a search bar and functionality to filter the child account list based on company name.
  • Updated the info notice to show more accurate terminology and slightly more specific helper text if there are no results.
  • Added unit test and Cypress test coverage.

Target release date 🗓️

06/10

Preview 📷

Screen.Recording.2024-05-24.at.9.44.03.AM.mov
Dev (Cloud Manager Parent Test Account) This Branch
Screenshot 2024-05-24 at 12 15 32 PM Screenshot 2024-05-24 at 12 16 47 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Log into our Cloud Manager parent user test dev account to test these changes.

Verification steps

(How to verify changes)

  • Confirm the x-filter in the network request is correct and all child accounts are displayed in alphabetical order.
  • Search for a company that is in the list and confirm the child accounts list updates to only show relevant results.
  • Search for a company name that is not in the list and confirm a message displays to inform the user there are no results.
  • Confirm loading and error states for /child-accounts request.
  • Confirm tests pass:
yarn test ChildAccountList SwitchAccountDrawer
yarn cy:run -s "cypress/e2e/core/parentChild/account-switching.spec.ts"

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@mjac0bs mjac0bs marked this pull request as ready for review May 24, 2024 16:45
@mjac0bs mjac0bs requested review from a team as code owners May 24, 2024 16:45
@mjac0bs mjac0bs requested review from AzureLatte, dwiley-akamai and hkhalil-akamai and removed request for a team May 24, 2024 16:45
Comment on lines -122 to -126
{(isSwitchingChildAccounts || isLoading) && (
<Box display="flex" justifyContent="center" width={'100%'}>
<CircleProgress mini size={70} />
</Box>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why we were setting up loading spinners in two different places, but I removed this and added all conditions to L67-72. The addition of isRefetching handles loading state for the search queries. Reviewers should check that they're not seeing any unexpected behavior in our handling of loading state.

@@ -67,7 +79,10 @@ export const ChildAccountList = React.memo(

if (childAccounts?.length === 0) {
return (
<Notice variant="info">There are no indirect customer accounts.</Notice>
<Notice variant="info">
There are no child accounts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not using "indirect customer", so updated the terminology here.

Copy link

github-actions bot commented May 24, 2024

Coverage Report:
Base Coverage: 82.29%
Current Coverage: 82.29%

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Code review is looking good; left some minor suggestions.

Note that I am currently unable to access alpha due to issues with my VPN access, so I will leave it to another reviewer to test functionality. Existing functionality looks good using MSW.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this test

Copy link
Contributor

@AzureLatte AzureLatte left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-05-28 at 9 29 03 AM
Screenshot 2024-05-28 at 9 30 47 AM

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Works great! 🎉

Screen.Recording.2024-05-28.at.9.48.37.AM.mov

I wonder if the user would benefit from having a clear button on the search field 💭

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels May 28, 2024
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Tests pass ✅
Code review ✅
Functionality (search, loading states, X-Filter, alphabetical sorting) ✅

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Nice work @mjac0bs 🚀

Screen.Recording.2024-05-29.at.10.42.05.PM.mov

@mjac0bs mjac0bs merged commit 1154b84 into linode:develop May 30, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Parent / Child Account
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants