feat: new UX for submitting changes #224
Conversation
| useEffect(() => { | ||
| if (nodeObject && originalNodeObject) { | ||
| const changesMade = | ||
| JSON.stringify(nodeObject) !== JSON.stringify(originalNodeObject); |
There was a problem hiding this comment.
JSON.stringify does not ensure that the strings will have the same shape. Let's use https://www.npmjs.com/package/fast-deep-equal for this. Also, no need for this to be a state, it can be a derived value
| </Button> | ||
| {/* Fab for submitting edits */} | ||
| {changesMade && ( | ||
| <Fab |
There was a problem hiding this comment.
could we add some background color for the Fab, just so that the button is not floating on top of the content?
| const [updateChildren, setUpdateChildren] = useState([]); // Storing updates of children in node | ||
| const [open, setOpen] = useState(false); // Used for Dialog component | ||
| const [changesMade, setChangesMade] = useState(false); // Used for displaying Fab |
There was a problem hiding this comment.
We can probably name this to isNewChange?
| @@ -54,6 +67,7 @@ const AccumulateAllComponents = ({ id, taxonomyName, branchName }) => { | |||
| }); | |||
| } | |||
| setNodeObject(duplicateNode); | |||
| setOriginalNodeObject(JSON.parse(JSON.stringify(duplicateNode))); // Deep copy | |||
There was a problem hiding this comment.
Why do we need the parse and stringify?
|
@diivi Can you resolve the conflicts? This PR seems to be stalled for a long time. Is there anything you need help with? |
Unfortunately, I don't have access to the device I set this project up on, so I'll have to set it up again some time 😞 |
Signed-off-by: eeshaanSA <[email protected]>
There was a problem hiding this comment.
@eeshaanSA Check out the comments and also run lint before you're committing.
| <ListAllEntryProperties | ||
| nodeObject={nodeObject} | ||
| setNodeObject={setNodeObject} | ||
| /> | ||
| {changesMade && ( |
There was a problem hiding this comment.
Please change the variable name to hasChanges. This makes it more understandable.
| minHeight : "50px", | ||
| borderRadius : "20px", |
There was a problem hiding this comment.
If possible, please make the dimensions in relative units. Will be easier when everything is being made responsive.
thanks. On it. |
|
@eeshaanSA thanks for taking this past the finish line! |
What
nodeObjectScreenshot
Fixes bug(s)