Skip to content

Conversation

@mj3cheun
Copy link

About

Add property to make graph responsive (that is make the graph position and size respond to changes in the viewport).

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 #62

@chachi
Copy link

chachi commented Apr 20, 2020

Any possibility of getting this merged? It'd be a huge win for the Dash-cytoscape integration to have full-screen, responsive canvas elements!

@mj3cheun
Copy link
Author

Just putting out there that I am willing to help get this PR up to date if there is any interest in merging.

Copy link

@xhluca xhluca left a comment

Choose a reason for hiding this comment

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

Hi Manfred! Thank you so much for opening this PR; I believe those additions will be highly valuable, and I see a lot of effort were put into making the Resize component. The code seems pretty lengthy, so I think it'd be great if you could post a GIF here to show how the end result looks like (I recommend Licecap for OS X).

There has been some problems with the tests failing recently, so I had to fix them on master. If you rebase this PR branch, more tests should pass.

I would love to merge this PR once we have addressed those points:

  • Resolve conflicts with dash_cytoscape/package.json
  • Rebasing (mentioned above)
  • Pass all the tests (make sure to eslint your new JS code and flake8/pylint the Python code if you haven't yet).
  • Log those changes in CHANGELOG.md

The following points would be "nice to have":

  • Add testing. You can refer to any of the existing tests, and either append to them, or add a new test file. It might a bit tough to thoroughly test this (you'd need to use selenium to change the viewport), but I think a simple test that checks if the app would render when resizable is True is sufficient.
  • Add a usage example, e.g. demos/usage-resize.py. Doesn't need to be anything really complicated. You can probably just have a single radioitem that switch between responsive or not.
  • Add the GIF (and a link to the file) at the end of the README.md

autounselectify: false,
autoRefreshLayout: true
autoRefreshLayout: true,
responsive: false
Copy link

Choose a reason for hiding this comment

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

Should responsive be true by default instead?

Copy link
Author

Choose a reason for hiding this comment

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

I chose to use responsive: false so that existing default functionality will remain unchanged and existing dash-cytoscape apps will not be unexpectedly altered by the new functionality. However if this is not an issue I can change it to responsive: true. Just let me know!

Copy link

Choose a reason for hiding this comment

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

You are right. In this case, let's keep it responsive: false, but in the readme.md, let's

  • add a subsection (right below "External layouts") that shows how activate it
  • make sure to have an usage-resize.py file in demos that shows how to resize
  • make a gif and add it at the end of the Gallery section in readme

@mj3cheun
Copy link
Author

mj3cheun commented Jun 2, 2020

Thanks for the feedback! I will begin work on updating the PR and integrating requested additions. I realize that the code is quite difficult to comprehend, so I can also provide a short document explaining how it works.

@xhluca
Copy link

xhluca commented Jun 2, 2020

Thanks, that would be great!

@mj3cheun mj3cheun mentioned this pull request Jun 12, 2020
6 tasks
@mj3cheun
Copy link
Author

mj3cheun commented Jun 12, 2020

Made a new PR with the updates #92

This PR can be closed

@xhluca
Copy link

xhluca commented Jun 17, 2020

Ok, I'll close this one.

@xhluca xhluca closed this Jun 17, 2020
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.

Viewport Responsiveness

4 participants