Skip to content

Fix multiple thoughts import overwrites siblings.#885

Merged
raineorshine merged 3 commits intocybersemics:devfrom
shresthabijay:fix-832
Oct 14, 2020
Merged

Fix multiple thoughts import overwrites siblings.#885
raineorshine merged 3 commits intocybersemics:devfrom
shresthabijay:fix-832

Conversation

@shresthabijay
Copy link
Contributor

@shresthabijay shresthabijay commented Oct 14, 2020

fixes #832

@shresthabijay
Copy link
Contributor Author

shresthabijay commented Oct 14, 2020 •
edited
Loading

@raineorshine There was a issue with the test you provided earlier. Thoughts updates needed to be merged with the previous thoughts. And the reason for overwriting siblings was that importJSON calculated contextIndexUpdates without adding children from already available contextIndex from the state. I have made necessary changes and it's working. Please review it. Thanks!

However I noticed that the tree is not re-rendering after import even in dev. Let me know if that is intended.

@raineorshine
Copy link
Contributor

raineorshine commented Oct 14, 2020

@raineorshine There was a issue with the test you provided earlier.

Yes, did you see the comment where pointed it out in the original issue? Anyway, thanks for fixing it!

And the reason for overwriting siblings was that importJSON calculated contextIndexUpdates without adding children from already available contextIndex from the state. I have made necessary changes and it's working. Please review it. Thanks!

Excellent, thanks!

However I noticed that the tree is not re-rendering after import even in dev. Let me know if that is intended.

No, we definitely want it to re-render after import.

raineorshine
raineorshine reviewed Oct 14, 2020
View reviewed changes
src/util/importJSON.ts
Comment on lines +135 to +137
const childrenUpdates =
contextIndexUpdates[contextEncoded]?.children ||
contextIndex[contextEncoded]?.children || []
Copy link
Contributor

@raineorshine raineorshine Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shresthabijay We can clean this up a bit with the optional chaining operator.

@raineorshine raineorshine merged commit 3b6ec68 into cybersemics:dev Oct 14, 2020
anmolarora1 pushed a commit to anmolarora1/em that referenced this pull request Mar 10, 2021
@raineorshine
Merge pull request cybersemics#885 from shresthabijay/fix-832
d08152e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raineorshine raineorshine raineorshine approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Importing multiple thoughts overwrites siblings

2 participants

@shresthabijay @raineorshine