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

Tidy up the Graph component (part #2) #383

Merged
merged 16 commits into from
Jan 28, 2020
Merged

Conversation

kinow
Copy link
Member

@kinow kinow commented Jan 23, 2020

These changes partially address #74 and #381, see also #302

Initial view of the Graph component when using this branch:

image

After dismissing the warning, and expanding the layout options:

image

Now left with a frozen graph, and dagre engine. While right has dismissed the warning, hierarchical layout, layout options hidden, and it is showing live data.

image

Tests above were done with Firefox 72.0.1 (64 bit) on Ubuntu LTS, using my home laptop.

@hjoliver it kinda works with 60.8. Will take another look tomorrow, but I suspect it's the Lumino widget and how it is handling updates and resizing.

For me, the /#/graph/families2 view worked fine.

image

But got the same issue reported by @hjoliver when using Lumino.

image

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Does not need tests (why? it's simply deleting or moving code).
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

@kinow kinow mentioned this pull request Jan 23, 2020
@kinow

This comment has been minimized.

@hjoliver
Copy link
Member

Need to fix the CSS styles that were "fixed" but broke the layout

What do you mean by "the layout" here - the graph layout, or more general page layout of the UI?

@kinow
Copy link
Member Author

kinow commented Jan 23, 2020

What do you mean by "the layout" here - the graph layout, or more general page layout of the UI?

The graph layout I think. The styles used by Cytoscape. More specifically, the zoom is wrong, and the height of the graph area also appears to be wrong.

But once these two are fixed, I think it should be ready for review.

@hjoliver
Copy link
Member

Technically the "graph layout" is the computed relative positions of all the graph nodes - the shape of the graph, sort of ... so maybe you mean the graph styling (and/or the viewport or whatever the term for that is)

@kinow
Copy link
Member Author

kinow commented Jan 23, 2020

Oh, yup. Graph styling, sorry. I am just trying to remember where each style is used, and what CSS styles need changing. Should be done by today/tomorrow.

@kinow
Copy link
Member Author

kinow commented Jan 23, 2020

I think I got too excited, so didn't squash the commits so that I can revert some if requested. Will add some comments to the new code, but here are the main changes:

  • There was some code from vue-cytoscape ported to the project.
  • Replaced string concatenation in the component by backtick (template literal)
  • Moved configuration objects to an index.js file
  • Moved component and related files to its own folder
  • Removed unnecessary files
  • Fixed typos and unnecessary or wrong CSS styles
  • Added comments to code
  • Used a v-switch and a v-select for layout controls (I think @oliver-sanders suggested it in some issue on github or message on riot 🎉 )
  • Used a v-expansion-panel to display the layout options only when requested (more visible space for the graph)
  • Set transparency to the minimap for the same reason above
  • Defined initial zoom level, and pan (position of graph)
  • Changed styles to use as much viewport as possible (both Graph view and on Lumino as widget)
  • Separated DOM elements, so that Lumino widgets are now able to display multiple Graph components
  • Added some code to clean up event listeners and Cytoscape objects
  • A few more changes here and there where I thought the code could be simpler or the IDE raised some warning

package.json Show resolved Hide resolved
public/index.html Show resolved Hide resolved
@@ -49,6 +49,7 @@

<v-menu
offset-y
v-if="$route.name === 'workflow'"
Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise "Add View" is available when accessing /#/tree/five or /#/graph/five — and it doesn't work.

src/components/cylc/graph/index.js Show resolved Hide resolved
src/styles/cytoscape/cytoscape-custom.scss Show resolved Hide resolved
src/components/cylc/graph/Graph.vue Show resolved Hide resolved
src/components/cylc/graph/Graph.vue Show resolved Hide resolved
src/components/cylc/graph/Graph.vue Show resolved Hide resolved
src/components/cylc/graph/Graph.vue Show resolved Hide resolved
src/components/cylc/graph/Graph.vue Show resolved Hide resolved
@kinow kinow requested review from hjoliver and oliver-sanders and removed request for hjoliver January 23, 2020 11:46
@kinow kinow marked this pull request as ready for review January 23, 2020 11:46
@oliver-sanders oliver-sanders added this to the 0.2 milestone Jan 23, 2020
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Code looks good, nice improvement.

  • Tested with 10 graph views on a small suite all displaying simultaneously.

    • Memory usage good.
    • System slightly laggy but nothing more than expected.
  • Tested with complex suite.

    • Working as expected.

@kinow
Copy link
Member Author

kinow commented Jan 23, 2020

Thanks for the review @oliver-sanders !

@kinow
Copy link
Member Author

kinow commented Jan 23, 2020

@oliver-sanders sorry, pushed two more commits ☝️

The first is to make the widget wrapper to invoke the resizeGraph() function of the Graph wrapped component, once the widget has been activated. This fixes the Firefox 60.8 issue reported by @hjoliver in #381 (this was the reason I started reviewing the Graph component code in the first place, and started this PR...)

The second commit is to fix a console error I noticed could happen when the user closed the widget while the layout was "running" (cytoscape.layout().run() more or less). There is nothing that the user or us can do about it, so the only option for that case is to ignore the error — though it's still possible to capture the error with the browser console by checking the option to break on any exception.

@oliver-sanders
Copy link
Member

The second commit is to fix a console error I noticed could happen when the user closed the widget while the layout was "running"

Interesting, I didn't see that.

Code looks good.

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Nice work @kinow . With this and previous changes, the cytoscape graphview is now a decent POC for demo purposes, until we settle on the proper way forward...

@hjoliver hjoliver merged commit fe33c61 into cylc:master Jan 28, 2020
@kinow kinow deleted the tidy-graph-1 branch March 19, 2020 01:02
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.

3 participants