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

FormField doesn't work with CustomComponent after updating to v2.11.3 #3898

Closed
hoshomoh opened this issue Mar 30, 2020 · 13 comments
Closed

FormField doesn't work with CustomComponent after updating to v2.11.3 #3898

hoshomoh opened this issue Mar 30, 2020 · 13 comments
Assignees
Labels
waiting Awaiting response to latest comments

Comments

@hoshomoh
Copy link

Expected Behavior

FormField should work with CustomComponent

Actual Behavior

After updating to v2.11.3, after typing one character in a custom text input, I can no longer type any other character. After spending some time debugging, I noticed that by adding the value property to the Form component fixes it.

This, however, introduces a warning.

Warning: Cannot update a component (`Form`) while rendering a different component (`TextInput`). To locate the bad setState() call inside `TextInput`, follow the stack trace as described in https://fb.me/setstate-in-render

URL, screen shot, or Codepen exhibiting the issue

Screenshot 2020-03-30 at 03 08 04

You can try it out here https://codesandbox.io/s/grommet-sandbox-vso4c. Try removing the value from From and notices that you can no longer type in the CustomComponent

Steps to Reproduce

  1. Add a CustomComponent to a FormField
  2. Make sure value property is not set for the Form
  3. Try to type in the CustomComponent, this wouldn't work.
  4. Add value to the Form component and you will be able to type
  5. Open your console, reload and type again
  6. You will see the warning message written above

Your Environment

  • Grommet version: 2.11.3
  • Browser Name and version: Chrome Version 80.0.3987.149 (Official Build) (64-bit)
  • Operating System and version (desktop or mobile): macOS Catalina version 10.15.3 (Desktop)
@forresthopkinsa
Copy link

Related: facebook/react#18178

@ericsoderberghp
Copy link
Member

ericsoderberghp commented Apr 1, 2020

Thanks for including the code sandbox, that helps to see what you are doing. I think the issue is that you don't have an onChange property in your custom FormFieldText. If I change

onChange={event => {
  onValueChange(event.target.value);
}}

to be onChange={onChange} and remove the value from Form, it works.

@hoshomoh
Copy link
Author

hoshomoh commented Apr 1, 2020

Thanks, @ericsoderberghp. Why should I have to pass an onChange my customComponent when I am already handling the change event? I guess Grommet is using the event internally for something.

The reason why I am leaving onValueChange is because I want the component to be controlled. This also seems to work.

onChange={e => {
    onChange(e);
    onValueChange(e.target.value);
}}

@ericsoderberghp
Copy link
Member

Who is onValueChange intended for?

The documentation for FormField component indicates that it is expecting onChange property, which aligns with how React works for <input /> elements. https://v2.grommet.io/formfield#component

@myktra
Copy link

myktra commented Apr 2, 2020

This is likely a related issue: #3910, with React 16.13.1 both in scope.

@hoshomoh
Copy link
Author

hoshomoh commented Apr 2, 2020

Who is onValueChange intended for?

The documentation for FormField component indicates that it is expecting onChange property, which aligns with how React works for <input /> elements. https://v2.grommet.io/formfield#component

onValueChange is intended for the parent component. In my case, I am using a controlled input which when value change I update the parent state, which is why I have passed an onValueChange the get called when the input changes.

Considering that Grommets explicitly wants an onChange with no custom code, I see no other way no approach this than to call both functions when the input changes.

@hoshomoh
Copy link
Author

hoshomoh commented Apr 2, 2020

But also when I do what you suggested I still get that warning in the console.

Screenshot 2020-04-02 at 18 33 08

import React, { useState } from "react";
import {
  Box,
  Button,
  CheckBox,
  Form,
  FormField,
  TextInput
} from "grommet";

const FormFieldText = ({
  name,
  placeholder,
  ariaLabel,
  value,
  onChange = () => {}
}) => (
  <Box fill="horizontal" direction="row" align="center">
    <TextInput
      plain
      type="text"
      name={name}
      placeholder={placeholder}
      aria-label={ariaLabel}
      value={value}
      onChange={onChange}
    />
  </Box>
);

const Main = () => {
  const [name, setName] = useState();

  return (
    <Form onSubmit={({ value }) => console.log("Submit: ", value)}>
      <FormField
        label="Full Name"
        name="full_name"
        placeholder="Max Mustarmann"
        value={name}
        onChange={e => setName(e.target.value)}
        component={FormFieldText}
      />
      <Button type="submit" label="Submit" primary={true} />
    </Form>
  );
};

export default Main;

@hoshomoh
Copy link
Author

hoshomoh commented Apr 6, 2020

Any update on this issue?

@ericsoderberghp ericsoderberghp self-assigned this Apr 6, 2020
@ShimiSun
Copy link
Member

I think this issue is fixed on the latest release, can you please verify?

@ShimiSun ShimiSun added the waiting Awaiting response to latest comments label May 27, 2020
@hoshomoh
Copy link
Author

Will do so today. Thanks for the update.

@hoshomoh
Copy link
Author

The initial error reported doesn't seem to happen anymore, however, I get a new console error regarding changing an uncontrolled input of type text to be controlled.

See sample component https://codesandbox.io/s/grommet-sandbox-g7564?file=/src/CustomFormField.js

@ericsoderberghp
Copy link
Member

The warning in the above sandbox is due to: const [name, setName] = useState(); not specifying an initial value. If I change that line to be const [name, setName] = useState(''); instead, the warning is removed. This is due to name now having an initial value of '' instead of undefined.

@hoshomoh
Copy link
Author

hoshomoh commented Jun 2, 2020

Thanks, @ericsoderberghp and @ShimiSun

@hoshomoh hoshomoh closed this as completed Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Awaiting response to latest comments
Projects
None yet
Development

No branches or pull requests

5 participants