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

feat(pie): support align to edge and labelLine mode #11381 #11632

Closed
wants to merge 7 commits into from

Conversation

Ovilia
Copy link
Contributor

@Ovilia Ovilia commented Nov 12, 2019

feat(pie): support align to edge and labelLine mode #11381

Problem and solution

In this PR, I proposed two new layouts for pie labels, aka aligning labels to 'lableLine' or 'edge'.

The default pie label layout may seem to be a little unorganized and not so beautiful, especially on devices with smaller resolutions.

Before:

image

After (two new layouts proposed):

image

image

API changes

The following are new options in this PR:

label: {
    alignTo: 'none', // 'none', 'labelLine', 'edge'. Works only when position is 'outer'. 'none' is for the previous design.

    // Closest distance between label and chart edge.
    // Works only position is 'outer' and alignTo is 'labelLine' or 'edge'.
    edgeMargin: 20,

    // Distance between text and labelLine. Previously, it's hard-coded as 3 in `labelLaout.js`.
    padding: 3
},
labelLine: {
    // Max length of length2 for 'labelLine' and 'edge' aligning.
    maxLength2: 200,

    // Min length of length2 for 'labelLine' and 'edge' aligning.
    minLength2: 20
}

The priority of edgeMargin is the highest. It is used to restrict text and label lines cannot go outside. maxLength2 and minLength2 are used to restrict the length of length2 so that it looks well on devices with both small and large resolutions.

Result and test

test/pie-alignTo.html provides test cases with different minLength2 and maxLength2.

Limitations and todo

  • textAlign is calculated from whether it's on the left or right side and cannot be changed by the user. We probably should provide the option to make the text align to the outer sides or the inner sides.
  • We should consider more options for the label design like Label enhancement on pie. #11381.

@Ovilia Ovilia changed the title WIP(pie): support align to edge and labelLine mode feat(pie): support align to edge and labelLine mode #11381 Nov 15, 2019
@pissang
Copy link
Contributor

pissang commented Nov 17, 2019

The demo works great, good job! Will review the code closely later.

About the configuration design, padding has already been used as a common property of label. https://www.echartsjs.com/zh/option.html#series-pie.label.padding I suggested use distance, or a more specific distanceToLabelLine.

@Ovilia
Copy link
Contributor Author

Ovilia commented Nov 19, 2019

The demo works great, good job! Will review the code closely later.

About the configuration design, padding has already been used as a common property of label. https://www.echartsjs.com/zh/option.html#series-pie.label.padding I suggested use distance, or a more specific distanceToLabelLine.

Sure. I will update the name after your review.

@@ -164,25 +231,38 @@ export default function (seriesModel, r, viewWidth, viewHeight, sum) {
var hasLabelRotate = false;
var minShowLabelRadian = (seriesModel.get('minShowLabelAngle') || 0) * RADIAN;

var seriesLabelModel = seriesModel.getModel('label');
var edgeMargin = seriesLabelModel.get('edgeMargin');
edgeMargin = parsePercent(edgeMargin, viewWidth);
Copy link
Contributor

Choose a reason for hiding this comment

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

edgeMargin in the outer scope is not used.

// Closest distance between label and chart edge.
// Works only position is 'outer' and alignTo is 'labelLine' or 'edge'.
edgeMargin: 20,
padding: 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra comma here

chart1.setOption(option1);
});
});
</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add another parameter to test edgeMargin? Also it will be great to record an interaction for testing.

@100pah
Copy link
Member

100pah commented Nov 21, 2019

(1)

In the pie-alignTo.html, slide the 'minLength2' in the "contoller panel" util all of the labels disappear in the first pie, and slide back. the labels will not be displayed any more.
I don't know whether it is a problem.
image

(2)

I have test this case: more than one pie charts in one echarts instance.

            function makeData() {
                var count = 10;
                var data = [];
                var text = '';
                for (var i = 0; i < count; i++) {
                    text += 'X';
                    data.push({
                        name: text + i,
                        value: Math.random()
                    });
                }
                return data;
            }

            var option0 = {
                series: [{
                    type: 'pie',
                    radius: '25%',
                    center: ['25%', '50%'],
                    data: makeData(),
                    // animation: false,
                    labelLine: {
                        minLength2: 50,
                        maxLength2: 10
                    },
                    label: {
                        // edgeMargin: 20,
                        position: 'outer',
                        alignTo: 'edge',
                        padding: 5
                    }
                }, {
                    type: 'pie',
                    radius: '25%',
                    center: ['75%', '50%'],
                    data: makeData(),
                    // animation: false,
                    labelLine: {
                        minLength2: 50,
                        maxLength2: 10
                    },
                    label: {
                        // edgeMargin: 20,
                        position: 'outer',
                        alignTo: 'labelLine',
                        padding: 5
                    }
                }]
            };

The result seams buggy.

image

(3)

I am not sure about the config edgeMargin. It seams define the gap between "a pie chart" and the dom container. But if consider there are more than one pie charts in one dom container, do we also need to consider the gap between different pie charts?
That is, do we need to define a rect area with left/right/top/bottom/width/height but not a edgeMargin? I am not sure. @Ovilia @pissang

@Ovilia
Copy link
Contributor Author

Ovilia commented Nov 22, 2019

@100pah (1) is not bug. It's because maxLength2 is set to be minLength2 when minLength2 is bigger than maxLength2. If you set maxLength2 to be a smaller number, the previous state will come back.

@Ovilia
Copy link
Contributor Author

Ovilia commented Nov 26, 2019

This PR is abandoned. New PR is #11715.

@Ovilia Ovilia closed this Nov 26, 2019
@Ovilia Ovilia deleted the fix-11381 branch November 26, 2019 10:30
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.

3 participants