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

TextArea and TextInput can't be pre-populated with value #3013

Closed
2 tasks done
davidbmaier opened this issue Jun 11, 2019 · 12 comments · Fixed by #3015
Closed
2 tasks done

TextArea and TextInput can't be pre-populated with value #3013

davidbmaier opened this issue Jun 11, 2019 · 12 comments · Fixed by #3015
Assignees
Milestone

Comments

@davidbmaier
Copy link

What package(s) are you using?

  • carbon-components - 10.3.0
  • carbon-components-react - 7.3.0

Detailed description

Since I've updated carbon-components to 10.3.0 and carbon-components-react to 7.3.0, both the TextInput and the TextArea components don't initially pass value props down to the actual elements - which means the fields cannot be prepopulated.

This does not happen in previous versions - to see this yourself, use the CodeSandbox below and use 10.2.0/7.2.0. There, the fields are populated with whatever you pass as value to the Carbon component.

I can see the issue in the latest Chrome and Firefox.

Steps to reproduce the issue

See this reduced code snippet in CodeSandbox.

Notes

When looking at the tree using the React DevTools, you can see the Carbon components receive the value prop, but the <input>/<textarea> elements always get an empty string for their value.

@jaknas
Copy link
Contributor

jaknas commented Jun 11, 2019

This is probably caused by this: useState which takes ' ' as default value, which is then output here. This is the same case in both TextArea and TextInput components.

My proposed solution would be:

  1. get the value prop from forwardRef
  2. change
    const [inputVal, setInput] = useState('');
    to something like this:
    const [inputVal, setInput] = useState(value ? value : '');

In TextInput component in storybook there's also something like defaultValue which changing isn't working in storybook as it is not used in the component. Maybe just to keep it consistent, expose value and communicate that it can be used as a default one - when using hooks it just depends on what you pass to useState, so shouldn't be hard.

@emyarod emyarod self-assigned this Jun 11, 2019
@emyarod
Copy link
Member

emyarod commented Jun 11, 2019

defaultValue is already spread as a prop for the input field but it's just not being taken as the initial state since the hooks refactor. given that we've been supporting defaultValue as a prop for multiple versions now, I'm going to leave the API unchanged and just pass defaultValue in as the initial value for the components

@smahale18
Copy link

smahale18 commented Jun 18, 2019

With this MR, is defaultValue made a required field? I don't have defaultValue prop and it breaks my code while rendering the code. Fixes only when I put defaultValue="".
Screen Shot 2019-06-17 at 11 05 13 PM

@Kleyu

@asudoh
Copy link
Contributor

asudoh commented Jun 18, 2019

@emyarod Would this be the case...? Thanks!

@gdelory
Copy link

gdelory commented Jun 18, 2019

Same here, I just ugraded to 7.3.1 thinking it would fix this, but I was using all my TextInput as fully controlled with value and onChange. Apparently this breaks all fully controlled TextInputs. Are we supposed to have both defaultValue and value now?

@emyarod
Copy link
Member

emyarod commented Jun 18, 2019

this is caused by defaultValue being unset I believe #3097

@gdelory
Copy link

gdelory commented Jun 18, 2019

Yeah I got that, but before 7.3, i.e. in 7.2, controlled text inputs were working fine with value only. We have hundreds of them, that's kind of a breaking change. So I'd like to know if it's going to stay like this and I have to update them all, or it's a temporary fix and 7.4 will put back what we had in 7.2 :)

@emyarod
Copy link
Member

emyarod commented Jun 18, 2019

I see, so it would be best modify the PR so that value or defaultValue is used (if supplied( is that right?

const [inputVal, setInput] = useState(value || defaultValue);

@gdelory
Copy link

gdelory commented Jun 18, 2019

Yes, I think it would help a lot of people! :)

@emyarod
Copy link
Member

emyarod commented Jun 18, 2019

got it! will make that change to #3097

@EAlexRojas
Copy link
Contributor

EAlexRojas commented Jun 18, 2019

Hi everybody,
The PR #3015 did not fix the issue.
It actually introduced a regression.

I created examples based in the code snipped in this issue, both use the same code but different versions of carbon-components / carbon-components-react:

I think that the main issue is that the value prop is being ignored.

value prop is part of others (https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/components/TextArea/TextArea.js#L51)
but then, the value passed to the actual textarea is an internal state (initialized now with 'defaultValue') (https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/components/TextArea/TextArea.js#L140)

Maybe, as in version 10.2.0 / 7.2.0, the value and defaultValue props should be just passed as is to the textarea (?)
or at least initialize the internal state with the value from the value prop (?)


In any case, as you can see in the code snippets, there is a regression here.
I hope this helps to find a solution.

@emyarod @asudoh

@emyarod
Copy link
Member

emyarod commented Jun 18, 2019

thanks for the codesandbox examples @EAlexRojas ! I believe this is addressed in #3097 now, just added a useEffect hook to make sure the props and state are synced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants