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

Editable: setting controlled value does not propagate into Editable's internal value #5670

Closed
1 of 3 tasks
kk21 opened this issue Mar 2, 2022 · 5 comments · Fixed by #5684
Closed
1 of 3 tasks

Editable: setting controlled value does not propagate into Editable's internal value #5670

kk21 opened this issue Mar 2, 2022 · 5 comments · Fixed by #5684
Labels
PR welcome 😇 Type: Bug 🐛 Something isn't working

Comments

@kk21
Copy link
Contributor

kk21 commented Mar 2, 2022

Description

Using React useState to control the value of Editable, calling setState does not seem to change the internal value of Editable.

Link to Reproduction

https://codesandbox.io/s/naughty-glade-ufqr78?file=/pages/index.tsx

Steps to reproduce

  1. The name state is set to "John" in useEffect once initially
  2. Click on the Editable text: "John"
  3. You can change, or not to change the Editable text.
  4. Press Escape key to abort.
  5. Editable shows placeholder text and the name state becomes the default empty string specified in useState("")

Chakra UI Version

1.8.6

Browser

Google Chrome 99

Operating System

  • macOS
  • Windows
  • Linux

Additional Information

No response

@kk21 kk21 added needs triage Issues and pull requests that need triage attention Type: Bug 🐛 Something isn't working labels Mar 2, 2022
@TimKolberger
Copy link
Contributor

Thank you for creating a reproduction! ✨

I updated the codesandbox with some logs to get more insight: https://codesandbox.io/s/chakra-ui-issue-5670-j0q3p5?file=/src/App.tsx

Seems like the previous value inside Editable is only updated, if Editable itself initiates the update.
It is not updated if the valueProp is changing.

const [value, setValue] = useControllableState({
defaultValue: defaultValue || "",
value: valueProp,
onChange: onChangeProp,
})
/**
* Keep track of the previous value, so if users
* presses `cancel`, we can revert to it.
*/
const [prevValue, setPrevValue] = useState(value)

The linked PR did not introduce the issue, it was already present in 1.7.2.

I think it is missing an Effect similar to this:

React.useEffect(() => {
  setPreviousValue(valueProp)
}, [valueProp])

@TimKolberger TimKolberger added PR welcome 😇 and removed needs triage Issues and pull requests that need triage attention labels Mar 2, 2022
@TimKolberger
Copy link
Contributor

I think a more straight forward solution would be to set setPrevValue(valueProp) in the onEdit callback, to use the most recent "previous" value in favor of the proposed effect.

@takethefake
Copy link
Contributor

takethefake commented Mar 3, 2022

Hej @TimKolberger @kk21

I also took a look at this and came to another approach which gives the user a little bit more control regarding the prevValue

I made prevValue a controllable prop that can be updated outside the scope of editable. this way you could update the prevValue in the useEffect as well.

by registering to onChangePrevValue you would get updates if the internal state would be updated.

But setting the value in the onEdit-callback seems a bit cleaner to me. will change my pr accordingly

@takethefake
Copy link
Contributor

When adjusting my PR I noticed one drawback to call setPrevValue in onEdit

when startWithEditView is true onEdit isn't called, since it is only called onFocus on the preview component.

which leaves us with the same error

takethefake added a commit to takethefake/chakra-ui that referenced this issue Mar 3, 2022
takethefake added a commit to takethefake/chakra-ui that referenced this issue Mar 5, 2022
takethefake added a commit to takethefake/chakra-ui that referenced this issue Mar 5, 2022
takethefake added a commit to takethefake/chakra-ui that referenced this issue Mar 5, 2022
@takethefake
Copy link
Contributor

@TimKolberger will now set the prevValue on the onFocus-Event on the input and preview component. Adjusted my PR accordingly.

takethefake added a commit to takethefake/chakra-ui that referenced this issue Mar 6, 2022
TimKolberger pushed a commit that referenced this issue Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR welcome 😇 Type: Bug 🐛 Something isn't working
Projects
None yet
3 participants