Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CyLeaflet AIO component #200

Merged
merged 48 commits into from Jan 22, 2024
Merged

Add CyLeaflet AIO component #200

merged 48 commits into from Jan 22, 2024

Conversation

cleaaum
Copy link
Contributor

@cleaaum cleaaum commented Nov 24, 2023

About

This PR adds a demo for the Cytoscape-Leaflet all-in-one component.

This AIO consists on a cytoscape graph displayed on top of a Leaflet map. Every movement (pan or zoom) on the cytoscape graph will update the map to move accordingly. All the logic for this to happen is in the file demos/assets/cyleaflet_clientside.js.

The coordinate conversion module proj4js is used to handle the dragging offset when the map is moved with a cytoscape object superimposed.

A demo of this component can be found in demos/usage_cy_leaflet_aio.py.

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.

dash_cytoscape/CyLeaflet.py Outdated Show resolved Hide resolved
window.dash_clientside = {};
}

window.dash_clientside.cyleaflet_utils = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nonblocking, but do these utils need to be exposed to the end user or do they just get used inside dash_clientside.cyleaflet? If the latter, they could just be local functions in this file (and transformElements could be inlined in dash_clientside.cyleaflet)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point -- changed all of these to local variables.

@@ -0,0 +1,88 @@
import proj4 from 'proj4'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about pulling in this whole package (the minified bundle is 86k if I download it by itself and its architecture doesn't look amenable to tree shaking) just to get one presumably very simple pair of converters. We can move forward now but let's investigate whether there's a simpler way to do this in the future.

Comment on lines 15 to 16
// Note: proj4.js is imported as an part of `external_scripts`
// in demos/usage-cy-leaflet-aio-ekl.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Note: proj4.js is imported as an part of `external_scripts`
// in demos/usage-cy-leaflet-aio-ekl.py

No longer the case, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Updated.

Copy link
Contributor

@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.

💃 A couple of small comments but I think this is good to merge, nice work!

Comment on lines 467 to 468
this.cyResponsiveClass = new CyResponsive(cy);
this.cyResponsiveClass.toggle(this.props.responsive);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! Good catch.

Comment on lines 32 to 34
window.dash_clientside.cyleaflet_utils = {
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
window.dash_clientside.cyleaflet_utils = {
};

No longer needed, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup

Copy link
Contributor

@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.

💃

@emilykl emilykl merged commit cdfea33 into master Jan 22, 2024
0 of 2 checks passed
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.

None yet

3 participants