Feature/node aspect ratio

[mikekucera]

General information

Associated issues: #89

Checklist

Author:

• [x] One or more reviewers have been assigned.
• [x] Automated tests have been included in this pull request, if possible, for the new feature(s) or bug fix.
• [x] The associated GitHub issues are included (above).
• [x] Notes have been included (below).

Reviewers:

• [x] All automated checks are passing (green check next to latest commit).
• [x] At least one reviewer has signed off on the pull request. Reviewers have two business days to review the pull request, after which the author may merge in the pull request unilaterally.

Notes

• Adds Node Size Orientation (AKA Aspect Ratio) controls.
• Adds new type of mapping: Dependant Mapping.
• Adds ability to unlock Node Size and set Width/Height independantaly.
• Adds Edge Transparency style section.
• Adds default mappings. For example if you go to Node Color, then select Color Gradient, then select an attribute, it automatically creates a default mapping. You don't need an extra click to create the mapping.
• Fixes several bugs (including #89).

[mikekucera]

Sorry, didn't check the mocha tests. I'll commit a fix shortly.

[d2fong]

Sorry, didn't check the mocha tests. I'll commit a fix shortly. @mikekucera This is related to some CX conversion tests I made. Do you mind me committing a fix since I am familiar with the tests/code?

[mikekucera]

Yes please, thanks!

[mikekucera]

Thanks Dylan.

[maxkfranz]

Looks good

A few discussion points:

• I think the small multiples for the aspect ratio input are great.
• Transparency should be multiplicative with the selection state so that they can coexist. E.g., an edge with opacity 0.5 set in the style should be 0.5 * ordinaryDeselectedOpacity when a selection is made that excludes the edge. You probably already accounted for this.
• At some point we should definitely have some tests for style application -- especially for the properties with new behaviour, e.g. dependent sizes. Now might not be the best time for that, since how these properties work may be in flux (and tests would be rewritten over and over). Eventually we should have vizmapper unit tests, scripts that generate images that can be visually verified against a set of known graph images, or both. For now, I think we can just tick the box given that we'll plan to add something more comprehensive later for testing.

[d2fong]

Yeah I am considering commenting out the tests I made because they are probably going to break every time someone adds something new. They help me develop CX export functionality but they probably slow everyone else down. Maybe I can just run those locally.

[mikekucera]

Transparency should be multiplicative with the selection state so that they can coexist. E.g., an edge with opacity 0.5 set in the style should be 0.5 * ordinaryDeselectedOpacity when a selection is made that excludes the edge. You probably already accounted for this.

No, I did not consider this. Now my attempts to get it working are not successful.

I'm not sure how/when cytoscape.js applies the transparency to the non-selected elements, but its not working when I apply the ordinaryDeselectedOpacity multiplier (which is 0.33 btw) in the vizmapper.calculate() method. Also, I need to know when some other element in the graph is selected, which I'm doing like this: !ele.cy().$('*:selected).empty() and that seems like it would be very bad for performance to do that for every edge. I also tried adding a listener to the vizmapper for selection events but that seems to get fired after calculate() is called. I need help with this, I'm not familiar enough with cytoscape.js and I'm not sure how to implement this.

[mikekucera]

Also there's a bug with the transparency slider which I'll fix now.

[maxkfranz]

I'm pretty sure it would just be something like this:

'opacity': ele => 0.333 * vizmapper.calculate(ele, 'opacity')

Here:

https://github.com/cytoscape/cytoscape-explore/blob/master/src/client/components/network-editor/index.js#L72

The unselected block overrides the prior, matching blocks, like CSS. If you do styling through the stylesheet, then everything 'just works'. Cytoscape knows when to recalculate style as long as you specify it declaratively in the stylesheet, so you shouldn't need any listeners etc. It's meant to be simple and automatic, as long as you follow CSS conventions.

You can see in the top blocks that all the vizmapper rules work in the same way. They're just being transformed from the vizmapper model to the declarative stylesheet spec.

On Wed, Oct 27, 2021 at 4:09 PM Mike Kucera @.***> wrote:

Transparency should be multiplicative with the selection state so that they can coexist. E.g., an edge with opacity 0.5 set in the style should be 0.5 * ordinaryDeselectedOpacity when a selection is made that excludes the edge. You probably already accounted for this.

No, I did not consider this. Now my attempts to get it working are not successful.

I'm not sure how/when cytoscape.js applies the transparency to the non-selected elements, but its not working when I apply the ordinaryDeselectedOpacity multiplier (which is 0.33 btw) in the vizmapper.calculate() method. Also, I need to know when some other element in the graph is selected, which I'm doing like this: !ele.cy ().$('*:selected).empty() and that seems like it would be very bad for performance to do that for every edge. I also tried adding a listener to the vizmapper for selection events but that seems to get fired after calculate() is called. I need help with this, I'm not familiar enough with cytoscape.js and I'm not sure how to implement this.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/cytoscape/cytoscape-explore/pull/90#issuecomment-953270716, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHRO44QJAKS5RJPUQU73CTUJBL6VANCNFSM5GYWQJ2A .

[maxkfranz]

Yeah I am considering commenting out the tests I made because they are probably going to break every time someone adds something new. They help me develop CX export functionality but they probably slow everyone else down. Maybe I can just run those locally.

If you want to do that, you can just put them in a different dir, e.g. tests-cx. Then a different script in package.json can still run your tests, e.g. npm run test:cx