Suggestion: Run layout after patch

[jdebr]

Hi,

We are using this component in a context where users can add/remove nodes dynamically, and it seems useful to re-run the layout after patching regardless of whether the layout options change. Just wondering what you thought about having this as an option and if there are any downsides to this that I am not considering. It could potentially be an opt-in configuration in case performance is a concern.

Happy to do a PR if this is something you think is useful. Thanks for providing the component.

[motin]

Looks like this has been asked and answered previously: https://github.com/plotly/react-cytoscapejs/issues/12

[jdebr]

I am doing something similar in my component with the cy reference (except only rerunning layout when the number of elements change, which works well for my use case). I was just thinking that it seems like a useful enough feature that it would be nice to have it wrapped in the component rather than having to do it imperatively outside the component, although I suspect it should be optional behavior as for large graphs it might cause performance issues.

In any case, this isn't an issue for me, so if there's no interest in adding this (or reasons it shouldn't be that I haven't considered) it can be closed. I just wanted to gauge interest in a PR to help out if it sounded useful.

[maxkfranz]

I think you'd often have to specify a custom diff implementation for options.layout. The default one doesn't fit, since it's a shallow equality comparison. Further, you may want to define a different diff operation for layouts than for elements.

Maybe props.diff(obj1, obj2) could take on an extra field parameter like props.diff(obj1, obj2, fieldName) where fieldName would specify the top-level Cytoscape options field. For example, you could have diff(layout1, layout2, 'layout') or diff(ele1, ele2, 'elements').

In this way, you could use different diff implementations for different fields. For typical object equality, you would probably only need shallow equality checks for elements whereas you may need deep equality checks for layouts. If you use something like Immutable.js, then you can always just use referential equality (=== or !==) for diff(), regardless of the field.

On Thu, Oct 17, 2019 at 9:28 AM Fredrik Wollsén [email protected] wrote:

Looks like this has been asked and answered previously: #12 https://github.com/plotly/react-cytoscapejs/issues/12

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/plotly/react-cytoscapejs/issues/38?email_source=notifications&email_token=AAHRO436QTLMDNVD5DWYIALQPBR7DA5CNFSM4JBDEQU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBQDBMQ#issuecomment-543174834, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHRO4ZUG7XZ6JLT7RAIGKDQPBR7DANCNFSM4JBDEQUQ .

[jdebr]

I think that could be useful; however I was thinking of applying and running the layout regardless of whether options.layout has changed.

In the current implementation, the layout is only applied and run if the layout.options has changed according to either the provided diff, or the default shallow diff, which is obviously true the first time the layout prop is passed in. However, at least for synchronous layouts, any new elements added after this initial run will not have the layout applied and will be placed at (0,0). I believe this will be true for asynchronous layouts as well since cy.layout() is only applied to the elements currently in the graph when it is called, but I haven't tested it to be sure.

I would think a frequent use case would be wanting to apply a layout whose options would never change, but would re-apply on any new nodes when they are added after the component is mounted.