giorgosart / react-easy-edit

Inline editing library for React
https://giorgosart.gitbook.io/react-easy-edit/
MIT License
251 stars 45 forks source link

Using editMode with default value #144

Closed cepres-nagi-ramadan closed 12 months ago

cepres-nagi-ramadan commented 2 years ago

Describe the bug When using editMode and value has some text. then you can't change text no more. It works fine if the value is empty.

To Reproduce Steps to reproduce the behavior: look to this code it reproduce the issue.

function App() { const [edit, setEdit] = useState(false); return ( <><EasyEdit type={Types.TEXT} value="I have custom buttons!" onSave={(val) => console.log(val)} editMode={edit} onCancel={() => setEdit(false)} /> <button onClick={() => setEdit(!edit)}>Edit Mode </> );

Expected behavior you can change text normally.

spacecat commented 1 year ago

I'm also experiencing this.

jonmadison-amzn commented 1 year ago

Updates? This may end up being a showstopper for us, with little time to dig into the source.

giorgosart commented 1 year ago

@spacecat @jonmadison-amzn @cepres-nagi-ramadan I think the problem here is that you have the "Save" button enabled which belongs to a child component of EasyEdit. When you click Save, you are trying to change the state of the parent component which is not working. Do you really need the save button? See below how i use this feature.

<h3>Toggle edit mode</h3>
                <button onClick={() => {this.setState({editMode: !this.state.editMode})}}>Toggle</button>
                <EasyEdit
                    type={Types.TEXT}
                    onSave={() => {
                      console.log("saved!")
                    }}
                    editMode={this.state.editMode}
                    hideCancelButton
                    hideSaveButton
                />
                <EasyEdit
                    type={Types.TEXT}
                    onSave={() => {
                      console.log("saved!")
                    }}
                    editMode={this.state.editMode}
                    instructions={"Toggle me!"}
                    hideCancelButton
                    hideSaveButton
                />
              </div>
giorgosart commented 1 year ago

I will take a look there is clearly a big with this.

pavelee commented 1 year ago

i am looking into this, will be back with feedback

pavelee commented 1 year ago

I prepared propose of solution -> https://github.com/giorgosart/react-easy-edit/pull/180

giorgosart commented 1 year ago

@spacecat @jonmadison-amzn @cepres-nagi-ramadan this is partially fixed in 1.17.1.

Thank you @pavelee for your contribution.

pavelee commented 1 year ago

@giorgosart I think we could close the issue.

giorgosart commented 12 months ago

@pavelee the reason i haven't closed this bug yet is because of the behaviour below. I didn't get a chance to look into it but the "Save" button becomes unclickable under certain scenarios. Ideally the Save button should not be there as it doesn't make sense to have an external button controlling the edit state of one or more EasyEdit components but it's still a bug. Let me know if you will get any time to look into this if you can.

https://github.com/giorgosart/react-easy-edit/assets/1062121/6a9a632e-c801-4a0d-9337-afea6be13afd

pavelee commented 12 months ago

I will look into it 👍, thanks for the great bug 🐛 example! 🙏

pavelee commented 12 months ago

@giorgosart I debugged the problem.

Here we have here problem with desynchronized internal and external state. EasyEdit cancel editMode onSave event, so we should keep the same logic with external state.

So to fix the issue we should update client code:

import React, { useState } from "react";
import "./App.css";
import EasyEdit, { Types } from "../lib/EasyEdit";

function App() {
  const [edit, setEdit] = useState(false);
  return (
    <>
      <EasyEdit
        type={Types.TEXT}
        value="text 1"
        onSave={(val) => {
          console.log(val);
          setEdit(false); // here we keep logic from EasyEdit, leave edit mode on save
        }}
        editMode={edit}
        onCancel={() => setEdit(false)}
      />
      <button onClick={() => setEdit(!edit)}>Edit Mode</button>
    </>
  );
}

export default App;

But here we could see that we got much bigger problem, because do we solve the issue we have here? As I understand we want to keep editMode as external state and toggle it. Not letting buttons to turn off editMode by them self.

Here example to better show my perspective:

import React, { useState } from "react";
import "./App.css";
import EasyEdit, { Types } from "../lib/EasyEdit";

function App() {
  const [edit, setEdit] = useState(false);
  return (
    <>
      <EasyEdit
        type={Types.TEXT}
        value="text 1"
        onSave={(val) => {
          console.log(val);
          setEdit(false);
        }}
        editMode={edit}
        onCancel={() => setEdit(false)}
      />
      <EasyEdit
        type={Types.TEXT}
        value="text 2"
        onSave={(val) => {
          console.log(val);
          setEdit(false);
        }}
        editMode={edit}
        onCancel={() => setEdit(false)}
      />
      <EasyEdit
        type={Types.TEXT}
        value="text 3"
        onSave={(val) => {
          console.log(val);
          setEdit(false);
        }}
        editMode={edit}
        onCancel={() => setEdit(false)}
      />
      <button onClick={() => setEdit(!edit)}>Edit Mode</button>
    </>
  );
}

export default App;

Do we here expect that clicking save on any button will leave editMode for every other input?

Maybe we should go in other direction, not letting save button to toggle editMode? So we can keep them in edit mode as long as parent component want.

pavelee commented 12 months ago

We could solve this cleaner by adding setEditMode to props, so EasyEdit can synchronize parent state by them self.

But still we should consider how EasyEdit should work with external editMode state.

giorgosart commented 12 months ago

@pavelee I agree that the client code should handle this, happy to close the issue now. In my opinion, when you use an external button to control the state the of one or more EasyEdits, the Save button should be hidden using the prop responsible for that.