Mpdx 8163 fix phone numbers input lag #1082

wjames111 · 2024-09-19T20:03:27Z

Description

Tickets

Tools - Fix Phone Numbers - input lag - Jira Ticket 8163
Tools - Fix Phone Numbers - consoler errors - Jira Ticket 8164
Tools - Fix Phone Numbers - Bulk Confirm Modal - Jira Ticket 8165
Tools - Fix Phone Numbers - Snackbar Warning - Jira Ticket MPDX-8173
Tools - Fix Phone Numbers - poorly sized contact - Jira Ticket MPDX-8172

Description of Each Ticket

Formik managing all state can cause significant lag with large numbers of entries. Ideally, implementing infinite scroll would improve performance by loading only a small portion of entries at a time. However, this doesn’t appear to be the case.
Console error p tag cannot be a descendent of a p tag
There is no modal for confirming bulk fixes to phone numbers, whereas all other tools have this feature.
Snackbar warning when Confirm All button does nothing
Deleting all numbers reveals poorly sized contact.

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 · 2024-09-19T20:04:12Z

Preview branch generated at https://MPDX-8163-fix-phone-numbers-input-lag.d3dytjb8adxkk5.amplifyapp.com

github-actions · 2024-09-19T20:05:29Z

Bundle sizes [mpdx-react]

Compared against a536cc4

Route	Size (gzipped)	Diff
`/accountLists/[accountListId]/tools/fix/phoneNumbers/[[...contactId]]`	147.76 KB	-101.75 KB

canac

Looks pretty good! Do you think you could fix the circle around the add button being an oval?

src/components/Tool/FixPhoneNumbers/Contact.tsx

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.test.tsx

src/components/Tool/FixPhoneNumbers/helper.ts

src/components/Tool/FixPhoneNumbers/helper.test.ts

canac · 2024-09-20T19:21:55Z

@wjames111 Everything is marked resolved, but did you make any changes? I looked at a few of the items, and it looks like it did when I reviewed last time.

wjames111 · 2024-09-20T19:23:50Z

@canac hmmm that's odd, I just rebased maybe I messed something up.

canac · 2024-09-20T19:15:26Z

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.tsx

@@ -124,53 +115,66 @@ const FixPhoneNumbers: React.FC<Props> = ({
 }: Props) => {
  const { classes } = useStyles();
  const { enqueueSnackbar } = useSnackbar();
-  const { appName } = useGetAppSettings();
+  const appName = process.env.APP_NAME ?? 'MPDX';


I think useGetAppSettings is a better way of doing this. Was there a reason you switched?

I noticed Fix Mailing Addresses was doing it this way, and I was trying to model off that tool as closely as possible as they're pretty similar.

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.tsx

canac · 2024-09-20T19:57:30Z

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.test.tsx

+    fireEvent.change(getByTestId('source-select'), {
+      target: { value: 'MPDX' },
    });


I think this could use userEvent.click also. You probably have to click on source-select and then click on getByRole('option', { name: "Item name" }).

So only issue with this is I'm getting, "unable to click element as it has or inherits pointer-events set to 'none'." Not sure the best way to get around this.

Is the selectbox disabled?

It doesn't appear to be

<Select className={classes.select} inputProps={{ 'data-testid': 'source-select' }} value={defaultSource} onChange={(event: SelectChangeEvent) => handleSourceChange(event) } size="small" > {sourceOptions.map((source) => ( <MenuItem key={source} value={source} data-testid="select-option" > {source} </MenuItem> ))} </Select>

Oh, the input has aria-hidden="true" on it. I think what you want to do is put the select and the "Default Primary Source" label in a <FormControl> like here. That will be better for accessibility too because "Default Primary Source" will be associated with the select box. Then in the test you can find it with getByRole('combobox', { name: 'Default Primary Source '}).

Nice this works, It does change the style of it a good amount. I think it works better though and like you said, it's more accessable. My only concern is it not looking like the other tools. Do you think I should wrap the bulk select with FormControl on the others as well?

src/components/Tool/FixPhoneNumbers/helper.ts

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.test.tsx

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.tsx

src/components/Tool/FixPhoneNumbers/Contact.tsx

canac · 2024-09-23T14:52:14Z

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.tsx

-                    </Select>
+                  <Box className={classes.defaultBox}>
+                    <FormControl size="small">
+                      <InputLabel shrink id="source-select" size="small">


Thanks for making this a FormControl! Looks like two elements have the id source-select and ids are supposed to be unique. You can probably remove them both unless you know of a reason to keep them. Also, I don't think that inputProps={{ 'data-testid': 'source-select' }} is needed anymore since we are finding the select input by its role.

canac · 2024-09-23T14:52:51Z

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.test.tsx

+    userEvent.click(primarySource);
+    userEvent.click(getByRole('option', { name: 'MPDX' }));
+
+    expect(getByTestId('source-select')).toHaveValue('MPDX');


Suggested change

expect(getByTestId('source-select')).toHaveValue('MPDX');

expect(primarySource).toHaveValue('MPDX');

Getting undefined here for some reason.

Honestly, I was also thinking that you could remove this line of the test. A more useful test could be to check that the confirm button as the text "Confirm 52 as MPDX". And I just realized that if "MPDX" is the initial value of that combobox, it would be better to select a different option so that the value actually changes. I feel like I'm stringing you along in asking for changes to the tests, but I promise that's not my intention.

Hahah no you're fine. I'd like to make it as good as I can. So your feedback is needed.

canac

I'm going to approve since my last two suggests are very minor. Great work with all of these improvements!

…lper.ts function.

wjames111 added On Staging Will be merged to the staging branch by Github Actions Preview Environment Add this label to create an Amplify Preview labels Sep 19, 2024

wjames111 self-assigned this Sep 19, 2024

wjames111 requested a review from canac September 19, 2024 20:03

canac reviewed Sep 19, 2024

View reviewed changes

wjames111 requested a review from canac September 20, 2024 18:58

wjames111 force-pushed the MPDX-8163-fix-phone-numbers-input-lag branch from 266833d to cbee27d Compare September 20, 2024 19:01

canac reviewed Sep 20, 2024

View reviewed changes

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.test.tsx Outdated Show resolved Hide resolved

canac reviewed Sep 20, 2024

View reviewed changes

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.test.tsx Outdated Show resolved Hide resolved

wjames111 requested a review from canac September 20, 2024 21:10

canac reviewed Sep 20, 2024

View reviewed changes

src/components/Tool/FixPhoneNumbers/FixPhoneNumbers.tsx Outdated Show resolved Hide resolved

canac reviewed Sep 20, 2024

View reviewed changes

src/components/Tool/FixPhoneNumbers/Contact.tsx Show resolved Hide resolved

wjames111 requested a review from canac September 23, 2024 14:39

canac reviewed Sep 23, 2024

View reviewed changes

canac approved these changes Sep 23, 2024

View reviewed changes

wjames111 added 5 commits September 23, 2024 12:59

Fix Phone Numbers, updates formik and state.

91a5cf8

Cleans up Fix Phone Numbers tests.

0027561

Fix Phone Numbers cleanup and review edits

dac6f0f

Fix for MPDX-8172 and MPDX-8173.

039635e

More PR review edits; useAppGetSettings, cleanup tests, simplifies he…

2a71034

…lper.ts function.

wjames111 added 2 commits September 23, 2024 13:00

Fixes error, p cannot appear as a descendent of p tag.

476dc3b

Fix Phone Numbers, update tests

e2ced21

wjames111 force-pushed the MPDX-8163-fix-phone-numbers-input-lag branch from 98bc3cd to e2ced21 Compare September 23, 2024 17:01

wjames111 merged commit 615138b into main Sep 23, 2024
17 of 18 checks passed

wjames111 deleted the MPDX-8163-fix-phone-numbers-input-lag branch September 23, 2024 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mpdx 8163 fix phone numbers input lag #1082

Mpdx 8163 fix phone numbers input lag #1082

wjames111 commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024 •

edited

Loading

canac left a comment

canac commented Sep 20, 2024

wjames111 commented Sep 20, 2024

canac Sep 20, 2024

wjames111 Sep 20, 2024

canac Sep 20, 2024

wjames111 Sep 20, 2024

canac Sep 20, 2024

wjames111 Sep 20, 2024 •

edited

Loading

canac Sep 20, 2024

wjames111 Sep 23, 2024

canac Sep 23, 2024

canac Sep 23, 2024

wjames111 Sep 23, 2024

canac Sep 23, 2024

wjames111 Sep 23, 2024

canac left a comment

	expect(getByTestId('source-select')).toHaveValue('MPDX');
	expect(primarySource).toHaveValue('MPDX');

Mpdx 8163 fix phone numbers input lag #1082

Mpdx 8163 fix phone numbers input lag #1082

Conversation

wjames111 commented Sep 19, 2024 • edited Loading

Description

Checklist:

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024 • edited Loading

Bundle sizes [mpdx-react]

canac left a comment

Choose a reason for hiding this comment

canac commented Sep 20, 2024

wjames111 commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjames111 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canac left a comment

Choose a reason for hiding this comment

wjames111 commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024 •

edited

Loading

wjames111 Sep 20, 2024 •

edited

Loading