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

delete tree if delete node is initiated for an empty tree #2806

Merged
merged 8 commits into from Jun 28, 2018

Conversation

@philippotto
Copy link
Member

commented Jun 26, 2018

Mailable description of changes:

  • When deleting the last node of a tree, that tree will not be removed automatically anymore. Instead, the tree will just be empty. To remove that active tree, the "delete" shortcut can be used again.

URL of deployed dev instance (used for testing):

Steps to test:

  • create a new tree and delete it with del again
  • test normal delete node functionality

Issues:


  • Ready for review

@philippotto philippotto self-assigned this Jun 26, 2018

@philippotto philippotto requested a review from daniel-wer Jun 26, 2018

philippotto added 4 commits Jun 26, 2018
@philippotto

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

@daniel-wer Should be ready for final review :)

@daniel-wer
Copy link
Contributor

left a comment

LGTM, very nice, works as described. I like that we got rid of the deleteTree side effect :)

.apply(SkeletonTracingReducer, deleteNodeAction)
.unpack();

t.deepEqual(

This comment has been minimized.

Copy link
@daniel-wer

daniel-wer Jun 28, 2018

Contributor

Shouldn't t.deepEqual(emptyTreeState, newState) be true here?

This comment has been minimized.

Copy link
@philippotto

philippotto Jun 28, 2018

Author Member

Unfortunately not :( The DiffableMap node collection creates an internal chunk when nodes are added, which is not removed again (it's just emptied). That's why the states differ. Ideally, the equal method would not compare the internal state of the DiffableMap, but I don't know how we can achieve this consistently without doing magic in an own compare-function..

This comment has been minimized.

Copy link
@daniel-wer

daniel-wer Jun 28, 2018

Contributor

I see, no big deal.

@@ -394,3 +369,36 @@ export const setTreeGroupAction = (groupId: ?string, treeId: number): SetTreeGro
groupId,
treeId,
});

// The following actions have the prefix "AsUser" which means that they

This comment has been minimized.

Copy link
@daniel-wer

daniel-wer Jun 28, 2018

Contributor

Nice, do you think moving the deleteTreeWithConfirmAction here as well would be sensible?

This comment has been minimized.

Copy link
@philippotto

philippotto Jun 28, 2018

Author Member

Yes, good point, I'll do that!

@philippotto philippotto added this to the Sprint 24a milestone Jun 28, 2018

@philippotto philippotto merged commit ddcfd82 into master Jun 28, 2018

1 check passed

ci/circleci: build_test_deploy Your tests passed on CircleCI!
Details

@normanrz normanrz deleted the delete-empty-tree-with-del branch Jul 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.