cytoscape / cytoscape-explore

Network visualization webapp.
MIT License
12 stars 4 forks source link

Feature/ndeximport cj #81

Closed d2fong closed 2 years ago

d2fong commented 2 years ago

Adding Import/Export to NDEx functionality to Cytoscape Explore

Summary of main changes

Controversial changes

These are changes that we would like feedback on. I will start comment threads at the particular lines so you can easily find them.

d2fong commented 2 years ago

Tests failing because there are 3 new visual properties to support

maxkfranz commented 2 years ago

Tests failing because there are 3 new visual properties to support

Could these be made into something like TODO comments instead of failing tests? The main value of the tests for PRs is to confirm that they're green in order to merge. If we merge this in, everyone else's PRs would be red no matter whether their changes broken things or not

d2fong commented 2 years ago

Tests failing because there are 3 new visual properties to support

Could these be made into something like TODO comments instead of failing tests? The main value of the tests for PRs is to confirm that they're green in order to merge. If we merge this in, everyone else's PRs would be red no matter whether their changes broken things or not

Yeah I am putting in a temporary fix to just ignore the new properties for now when dealing with cx import/export. @jingjingbic and I will add support for these new properties alongside the updated tests in a new branch. Is that ok?

maxkfranz commented 2 years ago

I've responded to the initial comments. I'll take a deeper look tomorrow