-
Notifications
You must be signed in to change notification settings - Fork 134
Closed
Labels
refactorRefactor without changing behaviorRefactor without changing behavior
Description
From my understanding of our current approach for
archiveThough, we're storing a react element in the redux state, hence making it non-serializable. So an undo/redo doesn't work as expected.
This approach doesn't seem ideal unless there's a strong reason for backing it.
redux.js.org/faq/organizing-state#can-i-put-functions-promises-or-other-non-serializable-items-in-my-store-state
reduxjs/redux#1248Yes, storing a component in the
stateseems like a bad idea! Will you propose a solution? Thanks!I inspected all the occurrences of the
alertreducer and it seems that we're storing the component only for thearchiveThoughtaction. These are the 2 solutions that I'd suggest -
- The component stored in the
valuerepresents - 1) the deleted thought, which is just a piece of text. 2) Theundo buttonwhich is used to undo thearchiveop. Since we're working on the generic undo approach which will coverundoing an archived thought, we can get rid of this redundant button, unless it serves some definitive purpose- Assuming that we need to have both the
undobuttons in place, we can add an undo button to the alert box based on the meta-data -alertType- And trigger the genericundo actionon its click.
This approach would save us from the state conflicts that we're likely face when the user hitsundofrom within the alert modal, and then tries to do the same from the toolbar.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
refactorRefactor without changing behaviorRefactor without changing behavior