[MLMD] Add support for many-to-many <EdgeCanvas> #44
Conversation
* Remove EdgeCanvas logic coupled to sleep timer and perform measurement of LineageView. * Sort Artifacts so they're laid out in Execution order. * Draw multiple EdgeCanvas elements to connect executions to Artifacts. * Adds <ControlledEdgeCanvas> for control of positioning relative to the card column and starting offset.
| axisVisible={false} | ||
| gridVisible={false} | ||
| labelsVisible={false} | ||
| pathColor={'#BDC1C6'} |
There was a problem hiding this comment.
Let's have the edgeColor be a css passed color.
| pathColor={'#BDC1C6'} | |
| pathColor={'currentColor'} |
Now you can simply assign a color on the html node, and the edge color should automatically update, all through css!
There was a problem hiding this comment.
That doesn't compute to the same color. https://screenshot.googleplex.com/qpLgGEoSFuB
There was a problem hiding this comment.
Well, you also need to update the CSS for LineChart to now contain this color. Basically moving the coloring onus to CSS instead of JS.
There was a problem hiding this comment.
Not sure I get how that would work - can you explain which files to modify?
Also I'm not sure how readable it is to have this component take in props about everything except the color which would be coming from a different file (I assume). What's the optimization here?
There was a problem hiding this comment.
Take a look at this:
I personally feel that colors shouldn't be props, all aesthetic changes should be done within CSS. This essentially allows a user to CSS theme their component however they feel, rather than having to programmatically change the color props of individual components. With this change, you can simply do:
.edgeCanvas {color: red} /* that's it!* /
.edgeCanvas:nth-of-type(5) {color: darkRed}There was a problem hiding this comment.
That won't work though. The CSS class is edgeCanvas_fbvqlg9 and generated by typestyle.
* Extract constant for the EdgeLine control point * Remove unnecessary usage of `!important`
|
Awesome, took a look, just one more comment! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avdaredevil 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 |
|
/hold cancel |
/area front-end
/priority p0
/assign @kwasi
/cc @avdaredevil
Fixes: #39
Background
As mentioned in #39, we currently fetch artifacts for all executions in the output executions column and connect them to the top-most execution.
https://screenshot.googleplex.com/ey9TJBUYbS1
This is wrong for 2 reasons.
If we really want to show the connections for the top-most execution and have the other executions be unconnected we should show just the first artifacts:
https://screenshot.googleplex.com/MwEwBi5tWw6
We can alternatively support targeting executions or connect the executions to the output artifacts. Since we don't have UX for targeting executions and want to present enough information to explore Lineage, this PR takes the approach of connecting the executions.
Description
This PR adds EdgeCanvas logic to support many-to-many connections between Output Artifacts and Output Executions. This allows us to show all executions connected to the correct artifacts, without having logic for execution selection, which may be added in the future.
https://screenshot.googleplex.com/AyQWkDHo6xp
https://screenshot.googleplex.com/0fNPiR4Upv9
It introduces a new Component
<ControlledEdgeCanvas/>which is built by the<LineageCardColumn>and given offsets to position the<EdgeLine>.Right now this component is coupled to the idea of OutputExecution to OutputArtifact edge lines, but it may either go away in the long-term (if we support execution targeting) or stay as a generic way of representing many-to-many edge connections.
Changes
componentDidMount().resizeevent handler to re-measure the LineageView width.<ControlledEdgeCanvas>for control of positioning relative to the card column and starting offset.tslinterrorsLineageActionBarwhere breadcrumbs could push the reset button off-screen.Verification
In Pipelines: https://6d1629804d56a867-dot-us-central2.pipelines.googleusercontent.com/#/artifact_types/Examples/artifacts/2
This change is