Clear Release Code Number when Release Code Type is NONE_OPTION

[dmkirshon]

Add:

• Handler for onValueChange Barcode Type Field prop so Barcode Number value is cleared on NONE_OPTION selection Type or Clearing the Type field.

Update:

• Dropdown Select to handle onValueChange from Dropdown Select Field
• Prop names to conform to "on" event handler convention

https://projectnewm.atlassian.net/browse/STUD-6

[github-actions]

Visit the preview URL for this PR (updated for commit ca004dc):

https://newm-artist-portal--pr467-stud-6-clear-release-p03zsugk.web.app

_{(expires Thu, 15 Feb 2024 09:36:07 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: ec3483d4c62309afc398865bfd6a9fc9e03e1d46}

[scandycuz]

The formatting here seems strange, haven't seen an if/else statement written this way before. Omitting brackets for single lines can be useful, but in this case, using the standard approach seems more conventional and readable. E.g.

if (newValue === null || newValue === undefined) {
  onValueChange?.("");
} else {
  onValueChange?.(newValue as string);
}

[scandycuz]

I haven't tested it, but this seems like it could also work:

onValueChange?.(newValue ?? "")

[dmkirshon]

Updating to use the nullish coalescing operator ??

[scandycuz]

Looking good but had a comment.

One minor update

[scandycuz]

LGTM, but had some comments.

[scandycuz]

The one change I'd make is to remove this call to props.onValueChange. E.g just:

onValueChange={ form.handleChange(props.name) }

I think that onValueChange (or potentially onChangeValue) is a better name than handleChange, since it starts with on, but the prop can essentially just do the same thing that the handleChange prop was doing, which was to serve as an alernative prop that accepts a string instead of an event. I don't believe there is a use case where it would be passed to the DropdownSelectField component.

[scandycuz]

Also, changing the handleBlur prop name to onBlur overrides the field.onBlur prop, however, field.onBlur and form.handleBlur appear to be the same, in which case it should be possible to omit it. The full field component would just be:

const DropdownSelectField: ForwardRefRenderFunction<
  HTMLInputElement,
  DropdownSelectProps
> = (props, ref) => (
  <Field name={ props.name }>
    { ({ field, form, meta }: FieldProps) => (
      <DropdownSelect
        { ...field }
        { ...props }
        ref={ ref }
        errorMessage={ meta.touched ? meta.error : "" }
        onChangeValue={ form.handleChange(props.name) }
      />
    ) }
  </Field>
);

[dmkirshon]

Made the changes and don't see any issues cropping up, good call. I think some of it was left over from previous issues or how the trigger events were being handled in connection with the DropdownSelect.tsx component

[scandycuz]

In the future, we could probably update the props for the field component to be more accurate as to which props it accepts, as apposed to just all of the DropdownSelectProps. For example, the onChange, onBlur, and onChangeValue props should be ommited since they wouldn't actually be passed to it and are instead handled internally using Formik.

[dmkirshon]

I updated the two component interfaces, it was bothering me too, let me know if it doesn't match up with what you envisioned.

We will need to update the other field/input components when they are touched. I believe they follow the same prop sharing pattern that you noted.

Edit: Had to update the interface reference for DropdownMultiSelect.tsx since it shared the DropdownSelect.tsx interface as well. We can clean up that components interface in a separate tasking as stated above.

[scandycuz]

It's looking great, but had just one last suggestion. I was realizing that although this works for our needs, since we're always using the DropdownSelect field with Formik, it could be confusing if a user was using it as a standalone controlled component, as they would have to know to pass an onChangeValue prop to the component instead of anonChange prop.

I was looking at the MUI docs for their AutoComplete component to see how they handled it, and it looks like they just define the onChange prop for the component as having the same definition as the onChange prop for useAutocomplete, which I think is a bit more straightforward. It would be good to match that functionality, just for completeness. The updated code would look like:

// DropdownSelectField.tsx

...

export interface DropdownSelectFieldProps {
  extends Omit<HTMLProps<HTMLInputElement>, "as" | "ref" | "onChange"> {
  ...
}

...

const DropdownSelectField: ForwardRefRenderFunction<
  HTMLInputElement,
  DropdownSelectFieldProps
> = (props, ref) => {
  return (
    <Field name={ props.name }>
      { ({ field, form, meta }: FieldProps) => (
        <DropdownSelect
          { ...field }
          { ...props }
          errorMessage={ meta.touched ? meta.error : "" }
          ref={ ref }
          onChange={ (event, newValue) => {
            form.handleChange(props.name)(newValue);
          } }
        />
      ) }
    </Field>
  );
};

...

// DropdownSelect.tsx
...
interface DropdownSelectProps extends DropdownSelectFieldProps {
  readonly errorMessage?: string;
  readonly onChange?: (
    event: SyntheticEvent<Element, Event>,
    newValue: string
  ) => void;
}

...

const DropdownSelect: ForwardRefRenderFunction<
  HTMLInputElement,
  DropdownSelectProps
> = (
  {
    ...
    onChange,
  },
  ref: ForwardedRef<HTMLInputElement>
) => {
  } = useAutocomplete({
    ...
    onChange: (event, newValue) => {
      onChange?.(event, newValue ?? "");
    },
    ...
  });
  ...
}

...

[scandycuz]

LGTM!