Skip to content

Conversation

@xhluca
Copy link

@xhluca xhluca commented Jul 3, 2020

About

This fixes the problem where creating a cyto.Cytoscape() without specifying elements would throw an error.

Description of changes

Pre-Merge checklist

  • The project was correctly built with npm run build:all.
  • If there was any conflict, it was solved correctly
  • All changes were documented in CHANGELOG.md.
  • All tests on CircleCI have passed.
  • All Percy visual changes have been approved.
  • Two people have 💃'd the pull request. You can be one of these people if you are a Dash Cytoscape core contributor.

Reference Issues

Closes #61

Other comments

@xhluca xhluca changed the title Set elements default to {}, yarn build Set Cytoscape.elements default to {} Jul 3, 2020
@xhluca xhluca changed the title Set Cytoscape.elements default to {} Explicitly set Cytoscape's elements default values Jul 3, 2020
@xhluca xhluca requested a review from alexcjohnson July 3, 2020 21:43
imageData: null,
responsive: false
responsive: false,
elements: {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not []?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both {} and [] are fine since 0.1, so I don't think it matters a lot.

Not according to the propType:

elements: PropTypes.arrayOf(PropTypes.object),

So either we should change this to something like

elements: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object])

Or we should change the default to []

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. I'll update the proptypes.

@xhluca
Copy link
Author

xhluca commented Jul 6, 2020 via email

@xhluca
Copy link
Author

xhluca commented Jul 6, 2020

By the way, this is the PR that added support for {}: plotly/react-cytoscapejs#2

Re-reading that PR, i realize that perhaps setting [] as default is a good idea, since {} is not the "preferred" way to define empty elements. Furthermore, we might end up going into a use case like:

app.callback(Output('cytoscape', 'elements'), Input('dropdown', 'value'), [State('cytoscape', 'elements'])
def add_new_node(elements):
  node = ...
  elements.append(node)
  return elements

in which case initializing an empty cytoscape graph with {} might pose problem.

@alexcjohnson
Copy link
Collaborator

alexcjohnson commented Jul 7, 2020

in which case initializing an empty cytoscape graph with {} might pose problem.

Default prop values don't come back in callbacks, so I think for a use case like this you'll still need to do something like return (elements or []) + [node]. But I agree that the default should reflect the preferred syntax.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

I haven't looked into the test failures, but assuming those get sorted out this looks good to me. 💃

@xhluca xhluca force-pushed the fix-empty-elements branch from 40b78a2 to cbb7516 Compare July 7, 2020 19:11
@xhluca xhluca changed the title Explicitly set Cytoscape's elements default values Explicitly set Cytoscape's elements default values, fix interactions test Jul 7, 2020
@xhluca xhluca merged commit c598d9b into master Jul 7, 2020
@xhluca xhluca deleted the fix-empty-elements branch July 7, 2020 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot read property of 'length' of undefined when updating from 0.0.5 to 0.1.1

3 participants