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 polyline edge layout for the tree series #7090 #11808

Merged
merged 10 commits into from Dec 20, 2019
Merged

Conversation

deqingli
Copy link
Member

@deqingli deqingli commented Dec 6, 2019

  • add edgeLayout in the TreeSeries.js.
  • draw the polyline edge in the TreeView.js.

Fix #7090.

@@ -139,6 +139,9 @@ export default SeriesModel.extend({
// the layout of the tree, two value can be selected, 'orthogonal' or 'radial'
layout: 'orthogonal',

// value can be 'polyline'
edgeLayout: 'curve',
Copy link
Contributor

@pissang pissang Dec 9, 2019

Choose a reason for hiding this comment

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

I think edgeShape is better. edgeLayout is more like the position of edge

Copy link
Member Author

@deqingli deqingli Dec 10, 2019

Choose a reason for hiding this comment

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

yeah, good suggestion!

@deqingli deqingli requested a review from 100pah Dec 16, 2019
ctx.lineTo(midPoint[0], point[1]);
}
}
}
Copy link
Contributor

@pissang pissang Dec 17, 2019

Choose a reason for hiding this comment

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

Top and bottom horizontal lines are better to be connected with the vertical line.

ctx.moveTo(); // top line end point
ctx.lineTo(); // top line connect point
ctx.lineTo(); // bottom line connect point
ctx.lineTo(); // bottom line end point

It will have a nicer corner than current if the line is thicker.
image

pissang
pissang previously approved these changes Dec 18, 2019
var location = parsePercent(shape.forkPosition, 1);
var midPoint = computeMidPoints(parentPoint, orient, ptMax, location);

ctx.moveTo(parentPoint[0], parentPoint[1]);
Copy link
Member

@100pah 100pah Dec 18, 2019

Choose a reason for hiding this comment

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

Wrong indent

function drawEdge(
seriesModel, node, virtualRoot, symbolEl, sourceOldLayout,
sourceLayout, targetLayout, group, seriesScope
) {
Copy link
Member

@100pah 100pah Dec 18, 2019

Choose a reason for hiding this comment

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

The indent is not good. The back brace should better align with the start of the sentence function,
and the rest of the code in the function should reduce 4 space.

}
var parentPoint = shape.parentPoint;
var orient = shape.orient;
var location = parsePercent(shape.forkPosition, 1);
Copy link
Member

@100pah 100pah Dec 18, 2019

Choose a reason for hiding this comment

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

The name location is not good.

var ptMin = [Infinity, Infinity];
var ptMax = [-Infinity, -Infinity];
var points = shape.childPoints;
for (var i = 0; i < points.length; i++) {
Copy link
Member

@100pah 100pah Dec 18, 2019

Choose a reason for hiding this comment

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

I think ptMin ptMax and the switch case of 'TB' 'BT' 'LR' 'RL' and computeMidPoints are not needed.
The code can be simplified if you wish. And some critical case need to be considered. For example:

buildPath: function (ctx, shape) {
    var childPoints = shape.childPoints;
    var parentPoint = shape.parentPoint;
    var childLen = childPoints.length;
    var firstChildPos = childPoints[1];
    var lastChildPos = childPoints[childLen - 1];

    if (!childLen || !parentPoint) {
        return;
    }

    if (childLen === 1) {
        ctx.moveTo(firstChildPos[0], firstChildPos[1]);
        ctx.lineTo(parentPoint[0], parentPoint[1]);
        return;
    }

    var forkDim = (orient === 'TB' || orient === 'BT') ? 1 : 0;
    var otherDim = 1 - forkDim;
    var forkPosition = parsePercent(shape.forkPosition, 1);
    var tmpPt = [];

    ctx.moveTo(parentPoint[0], parentPoint[1]);
    tmpPt[forkDim] = parentPoint[forkDim] + (firstChildPos[forkDim] - parentPoint[forkDim]) * forkPosition;
    tmpPt[otherDim] = parentPoint[otherDim];
    ctx.lineTo(tmpPt[0], tmpPt[1]);

    ctx.moveTo(firstChildPos[0], firstChildPos[1]);
    tmpPt[otherDim] = firstChildPos[otherDim];
    ctx.lineTo(tmpPt[0], tmpPt[1]);
    tmpPt[otherDim] = lastChildPos[otherDim];
    ctx.lineTo(tmpPt[0], tmpPt[1]);
    ctx.lineTo(lastChildPos[0], lastChildPos[1]);

    for (var i = 1; i < childLen - 1; i++) {
        var childPos = childPoints[i];
        ctx.moveTo(childPos[0], childPos[1]);
        tmpPt[otherDim] = childPos[otherDim];
        ctx.lineTo(tmpPt[0], tmpPt[1]);
    }
}

100pah
100pah approved these changes Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants