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

fix bug #7340 #11295

Closed
wants to merge 1 commit into from
Closed

fix bug #7340 #11295

wants to merge 1 commit into from

Conversation

yufeng04
Copy link
Contributor

No description provided.

@yufeng04 yufeng04 closed this Sep 20, 2019
@yufeng04 yufeng04 deleted the bug7340 branch September 20, 2019 17:05
@@ -0,0 +1,402 @@
<!--
Copy link
Member

Choose a reason for hiding this comment

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

The name of this file has typo

var chart2 = echarts.init(document.getElementById('plain2'), null, {});

var option2 = {
tooltip: {
Copy link
Member

Choose a reason for hiding this comment

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

wrong indent.

], function (echarts) {
var graphic = echarts.graphic;

var chart = echarts.init(document.getElementById('plain'), null, {
Copy link
Member

Choose a reason for hiding this comment

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

wrong indent

<div class="chart" id="plain"></div>
<h1>The style with legend symble should be series[i].itemStyle</h1>
<div class="chart" id="plain1"></div>
<h1>The style with legend symble should be legend.itemStyle</h1>
Copy link
Member

Choose a reason for hiding this comment

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

The description should better be more clear:
what style it will be (for example, what color). When running the test case manually,
I can not know what color of the legend border is expected.
It seems that it is not the same color as the border of the series shape in the second and third chart.

If need multiple line desription,
testHelper.createTest can be used, which support multiple line description
(take dataZoom-action.html and tmp-base.html as examples)

@@ -283,6 +283,14 @@ var LegendModel = echarts.extendComponentModel({
// 图例关闭时候的颜色
inactiveColor: '#ccc',

// 图例关闭时候的颜色
Copy link
Member

Choose a reason for hiding this comment

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

Use English please.
Although the previous comment is Chinese, we should change it to English gradually,
usually when modifying the file.

@@ -74,6 +77,25 @@ export default function (seriesType) {
// Set data all color for legend
dataAll.setItemVisual(rawIdx, 'color', singleDataColor);
}

if (!singleDataBorderColor) {
var borderColor = itemModel.get('itemStyle.borderColor')
Copy link
Member

Choose a reason for hiding this comment

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

For some historical reason, we have both 'barBorderColor' and 'borderColor'.


var borderColorAccessPath = (seriesModel.visualBorderColorAccessPath || 'itemStyle.borderColor').split('.');
var borderColor = seriesModel.get(borderColorAccessPath) // Set in itemStyle
|| seriesModel.getColorFromPalette(
Copy link
Member

Choose a reason for hiding this comment

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

should not repeat it, now that it both used in two places.


var borderColorAccessPath = (seriesModel.visualBorderColorAccessPath || 'itemStyle.borderColor').split('.');
var borderColor = seriesModel.get(borderColorAccessPath) // Set in itemStyle
|| seriesModel.getColorFromPalette(
Copy link
Member

Choose a reason for hiding this comment

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

should not repeat it, now that it both used in two places.

@@ -33,6 +33,15 @@ export default {

// FIXME Set color function or use the platte color
data.setVisual('color', color);

var borderColorAccessPath = (seriesModel.visualBorderColorAccessPath || 'itemStyle.borderColor').split('.');
Copy link
Member

Choose a reason for hiding this comment

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

For historical reason, we have both 'borderColor' and 'barBorderColor'.

@@ -41,16 +50,23 @@ export default {
data.setItemVisual(
idx, 'color', color(seriesModel.getDataParams(idx))
);
data.setItemVisual(
Copy link
Member

Choose a reason for hiding this comment

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

I prefer do not support set callback function for borderColor until it is really needed.
This kind of callback function brings some complexity.

@pissang
Copy link
Contributor

pissang commented Sep 23, 2019

Hi @yufeng04 . You need to add more detail in the title and description of pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants