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 unique key to transporter array elements #718

Merged

Conversation

sheckathorne
Copy link
Contributor

Description

Created a unique key for each Transporter by combining the EPAID and array index. This allows the user to apply all actions in the transporter drop-down menu without side-effects.

I found a bug where the tool-tips in the Transporter list displayed the zero-index value, i.e. 'view transporter 0 details' for transporter 1. I corrected the tests to reflect this change in tool-tip labels since the selectors are based on the index value.

Added a test to ensure that only one accordion expands when the same Transporter appears in the manifest and 'Details' is clicked.

Issue ticket number and link

Closes issue #620 [https://github.com//issues/620]

Checklist

  • [ x] I have added tests that prove my fix is effective or that my feature works
  • [ x] I have commented my code, particularly in hard-to-understand areas
  • [ x] I have made corresponding changes to the documentation
  • [ x] My changes generate no new warnings

…ach Transporter by combining the EPAID and array index. I also found a bug where the tooltips in the Transporter list displayed the zero-index value, i.e. 'view transporter 0 details' for transporter 1. I corrected the tests to reflect this change in tooltip labels. Also added a test to ensure that only one accordion expands when the same Transporter appears in the manifest and 'Details' is clicked.
@github-actions github-actions bot added the client Related to front end workings (React/Redux) label May 29, 2024
@dpgraham4401 dpgraham4401 self-requested a review May 29, 2024 11:59
@dpgraham4401
Copy link
Member

@sheckathorne Thanks for the PR, I should have time to look at it this week.

@dpgraham4401 dpgraham4401 changed the title in reference to issue #620 https://github.com/USEPA/haztrak/issues/62… Add unique key to transporter array elements May 29, 2024
@dpgraham4401
Copy link
Member

@sheckathorne Thanks for getting the ball rolling on this and submitting the PR. I've added a clientKey field to the transporter schema, the field is checked and, if undefined, a uuid is generated and used as a key for the transporter instance.

This was the only way I could think to fix the original bug while playing nice with react and not relying on the order/index of the transporter instance.

image

@github-actions github-actions bot added the dependencies project dependency problems or modifications label May 30, 2024
@dpgraham4401 dpgraham4401 merged commit ca64499 into USEPA:main May 30, 2024
10 checks passed
@sheckathorne sheckathorne deleted the manifest-transporters-unique-key branch May 30, 2024 19:23
@sheckathorne
Copy link
Contributor Author

After this commit I'm no longer able to add transporters to a manifest unless I change:

// TransporterSection.tsx 
 const transporters = transporterForm.fields;

  transporters.forEach((transporter, index) => {
    if (!transporter.clientKey) {
      // @ts-ignore
      manifestForm.setValue(`transporters[${index}].clientKey`, uuidv4());
    }
  });

to

// TransporterSection.tsx 
  const transporters: Array<Transporter> = manifestForm.getValues('transporters');
  transporters.forEach(transporter => {
    transporter.clientKey = uuidv4();
  })

however this re-introduces the problem of React re-rendering the entire table every time the transporters array is modified.

@dpgraham4401
Copy link
Member

dpgraham4401 commented May 31, 2024

Ah geez, here's the quick fix.

export function TransporterSection({ setupSign }: TransporterSectionProps) {
  const [, setSearchConfigs] = useHandlerSearchConfig();
  const [readOnly] = useReadOnly();
  const manifestForm = useFormContext<Manifest>();
  const { errors } = manifestForm.formState;
  const transporterForm = useFieldArray<Manifest, 'transporters'>({
    control: manifestForm.control,
    name: 'transporters',
  });
  const transporters = manifestForm.watch('transporters');

  transporters.forEach((transporter, index) => {
    if (!transporter.clientKey) {
      // @ts-ignore
      manifestForm.setValue(`transporters[${index}].clientKey`, uuidv4());
    }
  });
return ...

The difference is the change from const transporters = transporterForm.fields; to const transporters = manifestForm.watch('transporters');

@sheckathorne Do you want to submit a PR for this?

I've started looking at the react-hook-form useFieldArray documentation more, turns out the fields value returned from that hook has a unique id appended to each object intended to be used for this very thing (Doh!). I couldn't get the "Controlled Field Array" example (bottom of the page) example to rerender when a new transporter is appended, but that appears to be what we're going for as a long term fix.

@sheckathorne
Copy link
Contributor Author

Sure, I'll take a look at it today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Related to front end workings (React/Redux) dependencies project dependency problems or modifications
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants