Nested groups order logic #3206

yotamberk · 2017-06-25T15:29:38Z

fixes #3126 and refactor of nested groups logic

…tting

…groups-order

wimrijnders · 2017-07-05T08:40:37Z

For my understanding: the issue is that the nested groups appear in the wrong location, correct?

So, cook, clean house and shop should be placed directly under Lee.

Do I understand the issue like this?

wimrijnders

The fix is good. The rest of the comments is me being bloody-minded.

Edit: TIL that 'bloody-minded' is not a nice word and that I got the meaning wrong. Truly sorry about that, I meant that I seriously looked at the details.

wimrijnders · 2017-07-05T09:29:20Z

lib/timeline/component/ItemSet.js

+      if (me.groupsData && me.groupsData.length > 0) {
+        var groupsData = me.groupsData;
+        if (me.groupsData instanceof DataView) {
+          groupsData = me.groupsData.getDataSet()


Can groupsData be anything else than DataSet or DataView? Otherwise, there is no need for this test, DataSet has getDataSet() as well.

Same remark for line 1687.

I'll remove it

wimrijnders · 2017-07-05T09:44:57Z

lib/timeline/component/ItemSet.js

+          if (groupData.nestedGroups) {
+            if (groupData.showNested != false) {
+              groupData.showNested = true;
+            }


This block is a tautology if showNested is always defined and boolean.

I can imagine, though, that it might not be defined and this sets a default value. In that case if (!groupData.showNested) { will do - and I think it's actually more correct in this case, since current condition implies boolean.

groupData.nestedGroups is not required to be defined, hence the condition. This is the first place that basically defines it in case it isn't defined.

wimrijnders · 2017-07-05T09:46:22Z

lib/timeline/component/ItemSet.js

+                updatedNestedGroup.visible = false;
+              }
+              groupsData.update(updatedNestedGroup);
+            })


I hereby note that, when completely enabled, linting will yell at you about this. I propose that linting becomes compulsory when creating PR's - travis will fail on this.

Will be done

wimrijnders · 2017-07-05T09:51:08Z

lib/timeline/component/ItemSet.js

+            if (groupData.showNested != false) {
+              groupData.showNested = true;
+            }
+            groupData.nestedGroups.forEach(function(nestedGroupId) {


Observation: This is the actual fix, correct? LGTM.

wimrijnders · 2017-07-05T09:52:56Z

lib/timeline/component/ItemSet.js

+              }
+              groupsData.update(updatedNestedGroup);
+            })
+            groupsData.update(groupData);


I don't see why this call is necessary. It look superfluous to me. Why is it there?

Edit: Got it, it's for setting of showNested above. If you remove that assignment due to 'tautology', remove this as well.

It must be defined but I can make sure that the update is called once instead of calling update on every nestedGroup in the loop

wimrijnders · 2017-07-05T10:08:58Z

lib/timeline/component/ItemSet.js

@@ -1063,7 +1088,7 @@ ItemSet.prototype._onAddGroups = function(ids) {
  ids.forEach(function (id) {
    var groupData = me.groupsData.get(id);
    var group = me.groups[id];
-
+   


Pedantic overdrive: spaces in an empty line.
(I might be going over the top here, but I really do intend to review to the best of my capabilities)

wimrijnders · 2017-07-05T10:13:41Z

lib/timeline/component/ItemSet.js

@@ -1663,16 +1688,18 @@ ItemSet.prototype._onGroupClick = function (event) {
    groupsData = this.groupsData.getDataSet()
  }

-  group.showNested = !group.showNested;
+  var nestingGroup = groupsData.get(group.groupId)
+  nestingGroup.showNested = !nestingGroup.showNested;

  var nestedGroups = groupsData.get(group.nestedGroups).map(function(nestedGroup) {
    if (nestedGroup.visible == undefined) { nestedGroup.visible = true; }


Result of this overwritten by next line.

wimrijnders · 2017-07-05T10:15:25Z

lib/timeline/component/ItemSet.js


  var nestedGroups = groupsData.get(group.nestedGroups).map(function(nestedGroup) {
    if (nestedGroup.visible == undefined) { nestedGroup.visible = true; }
-    nestedGroup.visible = !!group.showNested;
+    nestedGroup.visible = !!nestingGroup.showNested;


Why not a direct assignment? nestingGroup.showNested is forced to boolean by line 1692.

true. It was left when it wasn't forced to be boolean

marcortw · 2017-07-08T13:49:26Z

lib/timeline/component/ItemSet.js

+              if (groupData.showNested == false) {
+                updatedNestedGroup.visible = false;
+              }
+              groupsData.update(updatedNestedGroup);


I think that the senderId should either be forwarded here or no event be triggered at all (if that is possible).

tfsarlo · 2017-08-03T14:38:26Z

is there a way to use multiple levels of groups and subgroups? so far i have seen only 2 levels.

…groups-order

…into nested-groups-order

yotamberk · 2017-08-12T15:22:49Z

@wimrijnders please review again =) I addressed the comments.
not my best PR... Good thing you've reviewed it well!

marcortw

Just two small proposed changes and one thing to think about. But besides that - very nice! I am eager to get this feature :-)

marcortw · 2017-08-12T16:26:02Z

lib/timeline/component/ItemSet.js

+
+      if (me.groupsData && me.groupsData.length > 0) {
+          var groupsData = me.groupsData.getDataSet();
+          console.log(me.groupsData instanceof DataView)


This log statement looks like a development leftover to me

marcortw · 2017-08-12T19:00:07Z

lib/timeline/component/ItemSet.js


-  group.showNested = !group.showNested;
+  var nestingGroup = groupsData.get(group.groupId)
+  nestingGroup.showNested = !nestingGroup.showNested;


When a nesting group is initialy loaded as 'open' (the implicit "showNested: true"), showNested is undefined. Negating it turns it to true which means expand, correct? This causes that an open loaded nesting group needs to be clicked twice. Adding this the line before should do the trick:

if (nestingGroup.showNested == undefined) nestingGroup.showNested = true;

Yeah... I had that line removed because I though it had no use. I'll re-add it

Ah, that was due to my review comment. Fine, so it needs to be in.

marcortw · 2017-08-12T19:19:33Z

lib/timeline/component/ItemSet.js


-  if (group.showNested) {
+  groupsData.update(nestedGroups.concat(nestingGroup));


I think it's a bit problematic to not have a sender here. But introducing something like a fake sender like 'nesting' wouldn't be clean either. My problem is that I catch the update event on the groups and do a lot of stuff afterwards. If there's no sender, I assume that the user modified the group, but in this case the user only cliked on the expand/collapse icon and didn't change the groups themselves. Let me illustrate this:

that.groups.on('update', function (event, properties, senderId) { switch (senderId) { case 'api': // do nothing / accept the changes break; case 'initialization': break; default: // item is supplied locally from the user/the library let currObj = that.groups.get(group.id); // --> update the changes to the API } });

With your changes, I may now perform a rather complicated object comparison to see if the changes were only in the visibility of the group, but that's a bit cumbersome. Nevertheless, I have no good idea on how to approach this and I can live with it.

I'll take another look at it now.

EDIT: If you look at the other updates in the file, you'll see that none of the methods send a senderId. I don;t think it should be sent here either.

wimrijnders · 2017-08-15T08:42:36Z

The travis test failing is at least partly due to #3313; it's not due to timeline. It's a unit test failing for network.

#3313 has been fixed to get rid of this testing error.

marcortw · 2017-08-18T17:59:12Z

@yotamberk just a quick question: Are we supposed to push the child group id to the parent's nestedGroups array and then update the parent group? Or the child group as well? I only updated the parent group and it seems fine except for that one odd effect.

I am not sure if my code is buggy or if there's still a little bug in this PR - I experience an effect which (sometimes?) seems to cause that the last added child group is not properly nested in its parent group. It seems like an update to a child group's nestedInGroup property happens only after a subsequent update to any parent group. My updates lazily sneak in through an API. I might be totally wrong though, which is why I'd like to know how exactly an update is supposed to be executed by the library user.

yotamberk · 2017-08-23T19:28:31Z

@marcortw Unfortunately this PR still does not resolve the problem of initially hidden nested groups so the issue you are dealing with is still open (there is an issue about it somewhere). It will need to be solved at some point. I'l currently trying to solve some other issues juggling my day job. I hope to be in holiday next week and pick up this issue aswell

* Fix sorting of nestedgroups when groups added after initial groups setting * Fix nesteded groups logic when adding and removing * Remove empty lines * Fix comments from PR * Remove spaces from empty line * Fix PR review comments

yotamberk added 4 commits June 24, 2017 21:07

Fix sorting of nestedgroups when groups added after initial groups se…

459008f

…tting

Fix nesteded groups logic when adding and removing

6119808

Merge branch 'develop' of https://github.com/almende/vis into nested-…

dd26d38

…groups-order

Remove empty lines

1992ab9

yotamberk added Confirmed Bug Timeline labels Jun 25, 2017

Merge branch 'develop' into nested-groups-order

e3bcc0f

wimrijnders self-requested a review July 4, 2017 06:51

wimrijnders suggested changes Jul 5, 2017

View reviewed changes

marcortw reviewed Jul 8, 2017

View reviewed changes

yotamberk added 2 commits July 17, 2017 00:01

Merge branch 'develop' into nested-groups-order

20d2b88

Merge branch 'develop' into nested-groups-order

5fc9549

marcortw mentioned this pull request Aug 4, 2017

Nesting _onGroupClick interference with html content #3333

Closed

yotamberk added 4 commits August 12, 2017 17:22

Merge branch 'develop' of https://github.com/almende/vis into nested-…

28ea36a

…groups-order

Fix comments from PR

951647d

Merge branch 'nested-groups-order' of https://github.com/yotamberk/vis …

4d7e4e8

…into nested-groups-order

Remove spaces from empty line

add5aac

marcortw suggested changes Aug 12, 2017

View reviewed changes

Fix PR review comments

dd6f035

wimrijnders approved these changes Aug 15, 2017

View reviewed changes

yotamberk merged commit 8c8f21d into almende:develop Aug 16, 2017

JobrianTrinidad pushed a commit to JobrianTrinidad/vis that referenced this pull request Aug 16, 2017

apply almende#3206

10cf26c

marcortw mentioned this pull request Sep 4, 2017

Nested groups semi-working add during runtime #3416

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested groups order logic #3206

Nested groups order logic #3206

yotamberk commented Jun 25, 2017

wimrijnders commented Jul 5, 2017

wimrijnders left a comment •

edited

Loading

wimrijnders Jul 5, 2017

wimrijnders Jul 5, 2017

yotamberk Aug 12, 2017

wimrijnders Jul 5, 2017 •

edited

Loading

yotamberk Aug 12, 2017

wimrijnders Jul 5, 2017

wimrijnders Jul 5, 2017

yotamberk Aug 12, 2017

wimrijnders Jul 5, 2017

yotamberk Aug 12, 2017

wimrijnders Jul 5, 2017 •

edited

Loading

yotamberk Aug 12, 2017

wimrijnders Jul 5, 2017

wimrijnders Jul 5, 2017

yotamberk Aug 12, 2017

wimrijnders Jul 5, 2017

yotamberk Aug 12, 2017

marcortw Jul 8, 2017 •

edited

Loading

yotamberk Aug 12, 2017

tfsarlo commented Aug 3, 2017

yotamberk commented Aug 12, 2017

marcortw left a comment

marcortw Aug 12, 2017

marcortw Aug 12, 2017

yotamberk Aug 12, 2017

wimrijnders Aug 15, 2017

marcortw Aug 12, 2017

yotamberk Aug 12, 2017 •

edited

Loading

wimrijnders commented Aug 15, 2017

marcortw commented Aug 18, 2017

yotamberk commented Aug 23, 2017


		if (group.showNested) {
		groupsData.update(nestedGroups.concat(nestingGroup));

Nested groups order logic #3206

Nested groups order logic #3206

Conversation

yotamberk commented Jun 25, 2017

wimrijnders commented Jul 5, 2017

wimrijnders left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcortw Jul 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfsarlo commented Aug 3, 2017

yotamberk commented Aug 12, 2017

marcortw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yotamberk Aug 12, 2017 • edited Loading

Choose a reason for hiding this comment

wimrijnders commented Aug 15, 2017

marcortw commented Aug 18, 2017

yotamberk commented Aug 23, 2017

wimrijnders left a comment •

edited

Loading

wimrijnders Jul 5, 2017 •

edited

Loading

wimrijnders Jul 5, 2017 •

edited

Loading

marcortw Jul 8, 2017 •

edited

Loading

yotamberk Aug 12, 2017 •

edited

Loading