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

Use this.sourceInfo.cloneModel instead #627

Merged
merged 1 commit into from
Nov 23, 2015

Conversation

nicklasb
Copy link
Contributor

Instead of again copying the original model value, use the already copied cloneModel that is exposed in the drag events.
This way, users can intercept and change the cloned data.

Instead of again copying the original model value, use the already copied cloneModel that is exposed in the drag events. 
This way, users can intercept and change the cloned data.
@nicklasb
Copy link
Contributor Author

See #628

@doillee
Copy link

doillee commented Nov 2, 2015

I need this fix too. I made the same fix in my local copy, however, it would be great if it can be part of official release.

@@ -169,7 +169,7 @@

// cloneEnabled and cross-tree so copy and do not remove from source
if (this.isClone() && this.isForeign()) {
this.parent.insertNode(this.index, angular.copy(nodeData));
this.parent.insertNode(this.index, this.sourceInfo.cloneModel);
Copy link
Member

Choose a reason for hiding this comment

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

this.sourceInfo.cloneModel seems like the right solution. However, in cloneModel() (L127), we check if cloning is enabled, as well as here (L172). This means we perform the check 2 times (in helper.js on L127 and here on L172).

Do you have any suggestions on how we can refactor this to prevent double checking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No..we check isClone, not cloneEnabled, right? Isn't it rather the comment that is wrong?
I'd say that is another commit.

@Voles
Copy link
Member

Voles commented Nov 4, 2015

See my feedback. Can you add a unit-test for this behaviour?

@Voles Voles added the reviewed label Nov 4, 2015
@Voles
Copy link
Member

Voles commented Nov 14, 2015

Any updates on this, @nicklasb? I'm not sure it's easy to add tests for this with the current codebase, but it would be nice to get your feedback on my comment. Thanks again!

@nicklasb
Copy link
Contributor Author

Hi, strange, I haven't seen any updates for this issue until now.
WRT unit test, I am not sure that I know how I would do that nicely.
And the test would only check that this fix is still applied.
Rather, it seems like the entire drag'n drop behavior should be tested, but that feels like a larger thing.

@Voles
Copy link
Member

Voles commented Nov 23, 2015

@nicklasb would it be possible to create a jsFiddle to reproduce the issue / effect. I'm not really sure what you try to achieve with the fix. Or maybe @doillee can provide additional information on his use case?

@nicklasb
Copy link
Contributor Author

Well, probably, but this is a very pretty reasonable and simple change.
I am not sure I have gotten through with my explanation, so I'll try again:

Currently, the original value, not cloneModel, is inserted into the target, which means that any changes to the cloneModel are disregarded and thus there is no way to intercept and affect what data is inserted into the target tree. The only thing you can insert is exactly the original.
This just cannot be correct in any circumstance? There is not much point in cloning the first time, then.

@Voles
Copy link
Member

Voles commented Nov 23, 2015

I got your point, thanks for explaining again. It would be helpful to add this to the documentation for future reference. Thanks!

Voles pushed a commit that referenced this pull request Nov 23, 2015
Use this.sourceInfo.cloneModel  instead
@Voles Voles merged commit 6ff77ac into angular-ui-tree:master Nov 23, 2015
@nicklasb nicklasb deleted the patch-1 branch November 23, 2015 20:00
Voles pushed a commit that referenced this pull request Dec 20, 2015
Voles pushed a commit that referenced this pull request Dec 20, 2015
Remove deprecated NOTE, which was fixed with #627
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants