tayv / Project-Bubblegum

React UI + controlled form components
https://bubblegumui.vercel.app
2 stars 0 forks source link

WrapperSelect: Duplicate Select component fields being registered by RHF #20

Closed tayv closed 1 year ago

tayv commented 1 year ago

The problem:

Example from the form submission console.log used in QuizTemplate page:

flatSelect: "third" // defaultValue set to "third"
flatselect: undefined // no defaultValue set
groupSelect: "third" // default value set to "third"
groupselect: "fourth" // default value set to "third" but then manually select a new option with value "fourth"

Example from WrapperSelect:


return (
    <>
      <Controller
        control={control}
        name={name}
        defaultValue={defaultValue}
        render={ ({ field: {onChange, value, ref, ...props} }) => (
          <>
            { !!label && <InputLabel type="standard" label={label} htmlFor={name} /> }
            { !!tip && <Tip text={tip} type="standard" />}
            <SelectRadix 
              onValueChange={onChange}
              value={value}
              forwardedRef={ref}
              placeholder={placeholder}
              itemOptions={itemOptions}
              {...props}
            />
          </>
         ) }  
      />
    </> 
  )

Potential causes:

asherccohen commented 1 year ago

First suggestion based on what I see here name="flatselect"

"name" is a prop that RHF uses to uniquely identify your form field (then uses it to map its value when you call onSubmit)

If two form fields share the same name there's a couple dfferent scenario that can happen, similar to what you describe. https://react-hook-form.com/api/useform/register/

This is not specific to RFH, it-s just how html forms work. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#name

tayv commented 1 year ago

Is the recommendation to also pass a name prop to the SelectRadix component?

Each component should already have a unique name. This is how I'm calling the two select components in test-form:

 <WrapperSelect 
    name="flatselect" 
    control={control} 
    placeholder="Select an option"
    defaultValue={defaultValues.flatSelect}
    itemOptions={ [
      {value:"first", labelText:"firstText", separator: false}, 
      {value:"second", labelText:"secondText", separator: true},
      {value:"third", labelText:"thirdText", separator: false}, 
      {value:"fourth", labelText:"fourthText", separator: true},
    ] }
  />

  <WrapperSelect 
    name="groupselect" 
    control={control} 
    label="This is a grouped list Radix Select"
    tip="Tip: This is a grouped list Radix Select"
    placeholder="Select an option"
    defaultValue={defaultValues.groupSelect}

    itemOptions={ [
        { groupLabel: "Group 1", 
          items: [
            {value:"first", labelText:"firstText", separator: false}, 
            {value:"second", labelText:"secondText", separator: true}
          ] 
        },
        { groupLabel: "Group 2", 
          items: [
            {value:"third", labelText:"thirdText", separator: false}, 
            {value:"fourth", labelText:"fourthText", separator: true}
          ] 
        }
      ] }
  />

When I try adding name={name} to Select.Rootin SelectRadix and then passing a prop to SelectRadix in WrapperSelect I get an error: 'name' is specified more than once, so this usage will be overwritten. and I can't remove the name prop on Controller or the application crashes with TypeError: Cannot read properties of undefined (reading 'substring').

Example of attempting to pass prop from WrapperSelect:

return (
    <>
      <Controller
        control={control}
        name={name}
        defaultValue={defaultValue}
        render={ ({ field: {onChange, value, ref, ...props} }) => (
          <>
            { !!label && <InputLabel type="standard" label={label} htmlFor={name} /> }
            { !!tip && <Tip text={tip} type="standard" />}
            <SelectRadix 
              name={name}
              onValueChange={onChange}
              value={value}
              forwardedRef={ref}
              placeholder={placeholder}
              itemOptions={itemOptions}
              {...props}
            />
          </>
         ) }  
      />
    </> 
  )
}

Here's a recording of how the WrapperSelect is still rendering twice after I try submitting the form: https://user-images.githubusercontent.com/48400779/227302609-b6bb02d7-081e-420d-a916-f09e16f706f3.mov

asherccohen commented 1 year ago

Double check, because in this file I see the same name being used.

https://github.com/tayv/Project-Bubblegum/blob/d6bb0080e4c859cf35718d2150cdd8d2523888b1/pages/forms/select-article.tsx

But maybe you're referring to another place, can you share the link?

"name" is a RHF only prop, so your WrapperSelect must drill it down to Controller but not to SelectRadix.

It is mandatory in RHF, which is why you get the crash.

On a side note, the "prop will be overridden" is due to when your types define the prop, as well as manually passing it to the component and spreading props.

type Props = {
name: string:
}

const MyComponent = (props) => {

//name is passed twice and spreading props will override the manual one
return <Select name={"field"} {...props}>
</Select>
}

Here's a quick quiz to test that:

const foo = { name: "joe" }

const bar = { name: "jack", ...foo }

What do you expect bar.name to be?

tayv commented 1 year ago

Sorry, yeah I was referring to the test-page which has a form submission button set up:

See line 309: https://github.com/tayv/Project-Bubblegum/blob/d6bb0080e4c859cf35718d2150cdd8d2523888b1/pages/forms/test-form.tsx

I just fixed the duplicate name on the select-article.tsx pg that you pointed out

asherccohen commented 1 year ago

Sorry, yeah I was referring to the test-page which has a form submission button set up:

See line 309: https://github.com/tayv/Project-Bubblegum/blob/d6bb0080e4c859cf35718d2150cdd8d2523888b1/pages/forms/test-form.tsx

I just fixed the duplicate name on the select-article.tsx pg that you pointed out

Ok I see a few mistakes here and patterns that can help avoiding them. I'll add comments.

asherccohen commented 1 year ago

Here's a couple of ideas about that test file:

Basically the file suffers of too much coupling and debugging will be harder and harder.

Then:

React Components render many more time than what we think and defining sub-components inline can be the cause of stale state and state-reset (the functional component gets re-created on every render, destroyed and attached again to the DOM)

Share state with props or Context (for static stuff), or elevate to a state manager for more dynamic state.

tayv commented 1 year ago

Fixed by refactoring the wrapper to the new Field component