"crossComparisonView" and "active" appear to be redundant. Are either of them required?

[julesjacobsen]

 {
            "groupId": "IMPC1",
            "groupName": "Group 1",
            "crossComparisonView": true, 
            "active": true,
            "entities": []
}

"crossComparisonView" and "active" appear to do the same thing - hide a group. I'm not sure isf that's what was intended, but if this feature is required a single property would be better. I'd go for "active" if there is a choice.

Secondly - what's the point of this anyway? If you don't want to display the data, don't put it in there to start with. What's the use-case for this?

[yuanzhou]

I had the same questions before. It's designed by another developer during the initial refactoring phase. According to the documentation, this is useful, if you not longer want that target group to be displayed within phenogrid and would like to retain the target group reference within the list.

Actually I noticed there's a difference between the crossComparisonView and active. Basically active can overwrite crossComparisonView and it also controls if Phenogrid will load the data initially. For example, having crossComparisonView set to false, but active as true, the data of that target group will be loaded, but that group won't show up in the selection menu.

[julesjacobsen]

OK, so that's how it works. But do you think this makes sense? Having an "active" switch makes no sense. I can see why you might want to load, but not display the data unless the user wants to toggle it on. But what's the point of putting a bunch of data into the json object which you then mark as being completely ignored?

Just having "crossComparisonView" and changing its name to "shown"/"visible"/"displayed" would have a much more obvious purpose. Two toggles for an on/off switch? I think that's overkill.

[yuanzhou]

@julesjacobsen I agree with you to be honest.

@harryhoch is that OK if I get rid of crossComparisonView and active from the config? We can always comment out those groups in code if need to retain the target group reference within the list. So all the groups specified in config/constructor will be the groups we want to display and can be turned on/off in selection menu. No need to use switches at all. I think I mentioned this to you several weeks ago but it was not high priority back that time, since Jules also mentioned this, maybe I should make the change now. Very easy.

[harryhoch]

@yuanzhou , @julesjacobsen , I trust your collective judgment on this one.

[yuanzhou]

https://github.com/monarch-initiative/phenogrid/commit/b259d7dbc6aa0433276087e0e085dc9a57daf65a

[julesjacobsen]

Cool, this looks way better now.