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

Do not store React component in state.alert #837

Closed
raineorshine opened this issue Aug 19, 2020 · 0 comments · Fixed by #840
Closed

Do not store React component in state.alert #837

raineorshine opened this issue Aug 19, 2020 · 0 comments · Fixed by #840
Assignees
Labels
refactor Refactor without changing behavior

Comments

@raineorshine
Copy link
Contributor

raineorshine commented Aug 19, 2020

#811 (comment):

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#1248

Yes, storing a component in the state seems like a bad idea! Will you propose a solution? Thanks!

I inspected all the occurrences of the alert reducer and it seems that we're storing the component only for the archiveThought action. These are the 2 solutions that I'd suggest -

  1. The component stored in the value represents - 1) the deleted thought, which is just a piece of text. 2) The undo button which is used to undo the archive op. Since we're working on the generic undo approach which will cover undoing an archived thought, we can get rid of this redundant button, unless it serves some definitive purpose
  2. Assuming that we need to have both the undo buttons in place, we can add an undo button to the alert box based on the meta-data - alertType - And trigger the generic undo action on its click.
    This approach would save us from the state conflicts that we're likely face when the user hits undo from within the alert modal, and then tries to do the same from the toolbar.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor without changing behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants