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

topology improvements: layers, bounds #5003

Merged

Conversation

christianvogt
Copy link
Contributor

@christianvogt christianvogt commented Apr 9, 2020

Fixes
https://issues.redhat.com/browse/ODC-2187

This PR addresses two topology improvements:

  • splitting bounds into separate position and dimension objects
  • adding layers to the model so that it's not hard coded

The reason for splitting bounds into 2 objects is because of how mobx works for optimizing react renders. If a node only needs the height and width but uses bounds, then anytime the position changes, the node will need to be re-rendered. This happens a lot on drag move of a node. By switching to two separate objects. The parent ElementWrapper component will re-render when position changes and only the node will re-render when the dimensions change.

Layers was a simple fix by adding a layers: string[] array to the Graph model object. This initializes the layers and removes the hard coded layers to become the default in case no layers are provided.

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

/assign @jeff-phillips-18

@openshift-ci-robot openshift-ci-robot added component/dev-console Related to dev-console approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 9, 2020
@@ -65,6 +65,7 @@ const graphModel: Model = {
id: 'g1',
type: 'graph',
layout: COLA_LAYOUT,
layers: ['bottom', 'groups', 'groups2', 'default', 'top'],
Copy link
Member

Choose a reason for hiding this comment

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

Could use constants from topology for most.

@@ -145,7 +145,7 @@ const TopologyViewComponent: React.FC<TopologyViewComponentProps> = ({ vis }) =>
id="collapse-blue"
label="Collapse Blue"
isChecked={collapseBlue}
onChange={setCollapseBlue}
onChange={(checked) => setCollapseBlue(checked)}
Copy link
Member

Choose a reason for hiding this comment

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

why these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Warnings were logged in the console because onChange actually has 2 params and the state functions only accept 1.

Copy link
Member

@jeff-phillips-18 jeff-phillips-18 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 70ae2b7 into openshift:master Apr 10, 2020
@spadgett spadgett added this to the v4.5 milestone Apr 14, 2020
@christianvogt christianvogt deleted the topology-bounds branch August 20, 2020 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants