-
Notifications
You must be signed in to change notification settings - Fork 265
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 graph model generator and update UI components #750
Conversation
18bb6d3
to
6aa4b29
Compare
@@ -93,7 +108,11 @@ export default class Graph extends PureComponent { | |||
<VXGraph | |||
graph={{ links, nodes }} | |||
nodeComponent={c => ( | |||
<Node onClick={n => console.log('onClick', n)} {...c.node} /> | |||
<Node | |||
onClick={isSubGraph ? onClickStep : onClickTask} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the three isSubGraph
conditionals be in a single block outside the jsx ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what did you have in mind, something like this?
+let nodeOnClick = onClickTask;
+let nodeOnClickStep = onClickStep;
+let linkComponent = NodeLink;
+if (isSubGraph) {
+ nodeOnClick = onClickStep;
+ nodeOnClickStep = undefined;
+ linkComponent = () => null;
+}
...
nodeComponent={c => (
<Node
- onClick={isSubGraph ? onClickStep : onClickTask}
+ onClick={nodeOnClick}
- onClickStep={isSubGraph ? undefined : onClickStep}
+ onClickStep={nodeOnClickStep}
{...c.node}
/>
)}
- linkComponent={isSubGraph ? () => null : NodeLink}
+ linkComponent={linkComponent}
not sure it's an improvement
- add code to generate graph data model from set of Tekton resources representing a PipelineRun - simplify graph model to remove unecessary fields - add TaskRun / step status to nodes - update UI components to support dynamically redrawing the diagram - add click handlers so consumers can hook into graph interaction (Task / step selected)
6aa4b29
to
03781fa
Compare
@steveodonovan thanks for the feedback, addressed it all bar the one still open above. Also refactored the main function in buildGraphData.js to extract the individual stages and hopefully make it a little more readable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: steveodonovan 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 |
Changes
#675
representing a PipelineRun
selected)
Note: most of the diff (~600 lines) is sample data (examples/*.json) and can be ignored.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide
for more details.