MPDX-7420 - Fix Email Addresses - add pagination

[wrandall22]

Description

Jira

This PR adds an infinite list integration with the Fix Email Addresses tool. It does some improvements to the InfiniteList component to remove the flicker and allow for removing the gray background when we don't want it.

Checklist:

• I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
• I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
• I have requested a review from another person on the project

[github-actions]

Bundle sizes [mpdx-react]

Compared against 103d35e

Route	Size (gzipped)	Diff
/accountLists/[accountListId]/tools/fixEmailAddresses/[[...contactId]]	155.37 KB	+18.97 KB

[wrandall22]

FWIW, I tried upgrading react-virtuoso to 4.10.0, but I still get a flicker.

[github-actions]

Preview branch generated at https://MPDX-7420-fix-email-addresses-add-pagination.d3dytjb8adxkk5.amplifyapp.com

[canac]

Let's remove the !loading check so that when a new batch of people are being fetched, the entire list doesn't disappear.

We also need to configure the cache policy for people so that new results are merged into the existing array.

Also, I noticed that the "Showing x of x" text is incorrect.

Since all those suggestions are on unchanged lines, I can't add comments in GitHub, so here's a diff of the changes I'm suggesting.

diff --git a/src/components/Tool/FixEmailAddresses/FixEmailAddresses.tsx b/src/components/Tool/FixEmailAddresses/FixEmailAddresses.tsx
index 4061facbb..2589f9de2 100644
--- a/src/components/Tool/FixEmailAddresses/FixEmailAddresses.tsx
+++ b/src/components/Tool/FixEmailAddresses/FixEmailAddresses.tsx
@@ -320,7 +320,7 @@ export const FixEmailAddresses: React.FC<FixEmailAddressesProps> = ({
 
   return (
     <Container>
-      {!loading && data && dataState ? (
+      {data && dataState ? (
         <FixEmailAddressesWrapper container>
           <Grid item xs={12}>
             <Typography variant="h4">{t('Fix Email Addresses')}</Typography>
@@ -415,9 +415,12 @@ export const FixEmailAddresses: React.FC<FixEmailAddressesProps> = ({
                 <Box width="100%" display="flex" justifyContent="center">
                   <Typography>
                     <Trans
-                      defaults="Showing <bold>{{value}}</bold> of <bold>{{value}}</bold>"
+                      defaults="Showing <bold>{{value}}</bold> of <bold>{{total}}</bold>"
                       shouldUnescape
-                      values={{ value: data.people.nodes.length }}
+                      values={{
+                        value: data.people.nodes.length,
+                        total: data.people.totalCount,
+                      }}
                       components={{ bold: <strong /> }}
                     />
                   </Typography>
diff --git a/src/lib/apollo/cache.ts b/src/lib/apollo/cache.ts
index 307a29f11..2e962a634 100644
--- a/src/lib/apollo/cache.ts
+++ b/src/lib/apollo/cache.ts
@@ -72,6 +72,7 @@ export const createCache = () =>
           contacts: paginationFieldPolicy,
           donations: paginationFieldPolicy,
           financialAccounts: paginationFieldPolicy,
+          people: paginationFieldPolicy,
           tasks: paginationFieldPolicy,
           userNotifications: paginationFieldPolicy,
           // Ignore the input.pageNumber arg so that queries with different page numbers will

[canac]

I'm seeing a tiny amount of double scrolling. I think this calc may need to be adjusted to make the list slightly smaller.

Double.scrolling.mov

[wrandall22]

Changing that value can increase the double scroll but not get rid of it entirely it seems. If I hide the scrolling on overflow-y on the <div class="css-1wkgudm"> I can get rid of the extra scroll bar. However, no height changes I've tried in the DOM make it go away.

[canac]

Double scrolling issues are the bane of my existence. I can't even reproduce it anymore, so I think this is OK as-is.

[wrandall22]

Let's remove the !loading check so that when a new batch of people are being fetched, the entire list doesn't disappear.

This doesn't seem to impact this actually. Changing the values for increaseViewportBy makes it smoother. The larger the numbers, the less of the problem there is, but the longer the initial load takes as well.

Update: It does, I was getting bad tests due to not having more than 25 results.

[wrandall22]

We also need to configure the cache policy for people so that new results are merged into the existing array.

This is causing duplicate rows in the tests and breaking them. Specifically, in change second email for second person to primary then delete it the getByTestId('starIcon-testid2-0') returns multiple rows.

[canac]

We also need to configure the cache policy for people so that new results are merged into the existing array.

This is causing duplicate rows in the tests and breaking them. Specifically, in change second email for second person to primary then delete it the getByTestId('starIcon-testid2-0') returns multiple rows.

Try updating defaultGraphQLMock to:

const defaultGraphQLMock = {
  GetInvalidEmailAddresses: {
    people: {
      nodes: mockInvalidEmailAddressesResponse,
      pageInfo: {
        hasNextPage: false,
      },
    },
  },
};

Without this hasNextPage is getting mocked to true, then it's loading another page, which will contain the same mocked data.

[wrandall22]

That seems odd to me that it would default mock a boolean to true instead of false

[canac]

That seems odd to me that it would default mock a boolean to true instead of false

It's technically random with a deterministic seed. So you're happening to get a true random value here. But since the test requires false to work, it's better to make that explicit.

[dr-bizz]

const defaultGraphQLMock = {
GetInvalidEmailAddresses: {
people: {
nodes: mockInvalidEmailAddressesResponse,
pageInfo: {
hasNextPage: false,
},
},
},
};

You could run the first and second queries as true and then switch them to false. Bill, I have seen you use this elsewhere.

const defaultGraphQLMock = () => {
  let queryResponse;
  if (queryResponse < 2) {
    queryResponse = {
      people: {
         nodes: mockInvalidEmailAddressesResponse,
         pageInfo: {
           hasNextPage: true,
         },
       }
    }
   else {
     queryResponse = {
       people: {
         nodes: mockInvalidEmailAddressesResponse,
         pageInfo: {
           hasNextPage: true,
         },
       }
     };
   }
   queryResponse++;
   return queryResponse;
}

You can also change the mockInvalidEmailAddressesResponse to be different if you don't want it to return the same results.

[canac]

I'm not sure that the last commit with the contacts list helps. I recommended moving it out of this PR, but I'm going to go ahead and approve because the rest looks great!

[canac]

Double scrolling issues are the bane of my existence. I can't even reproduce it anymore, so I think this is OK as-is.

[wrandall22]

Getting a CORS issue all the sudden on the preview environment, but don't think that should be related to any changes here.