You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2020/07/13 10:11:12 UTC
[GitHub] [incubator-echarts] plainheart opened a new pull request #12961: feat: more configurable styles for lineStyle.
plainheart opened a new pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [ ] bug fixing
- [x] new feature
- [ ] others
### What does this PR do?
<!-- USE ONCE SENTENCE TO DESCRIBE WHAT THIS PR DOES. -->
Support for configuring the `lineCap`(named of `cap`), `lineJoin`(named of `join`), `lineDashArray`(named of `dashArray`) and `lineDashOffset`(named of `dashOffset`) for `lineStyle`.
TODOs:
- [ ] Support for `border`
### Fixed issues
N/A now.
## Details
### Before: What was the problem?
<!-- DESCRIBE THE BUG OR REQUIREMENT HERE. -->
`lineStyle` lacks `lineStyle.dashArray`, `lineStyle.dashOffset`, `lineStyle.cap`, `lineStyle.join` and etc.
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
### After: How is it fixed in this PR?
<!-- THE RESULT AFTER FIXING AND A SIMPLE EXPLANATION ABOUT HOW IT IS FIXED. -->
Added these options listed above.
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
## Usage
### Are there any API changes?
- [x] The API has been changed.
<!-- LIST THE API CHANGES HERE -->
Added some new options:
`lineStyle.dashArray`, `lineStyle.dashOffset`, `lineStyle.cap`, `lineStyle.join`
### Related test cases or examples to use the new APIs
`test/line-style.html`
## Others
### Merging options
- [ ] Please squash the commits into a single one when merge.
### Other information
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] plainheart commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r470588142
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
The second issue:
if we make `borderDashArray/dashArray(line)` be a value of `borderType/type(line)`,
the `ITEM_STYLE_KEY_MAP` in `model/mixin/itemStyle.ts` will miss the property `borderDash` and
the `LINE_STYLE_KEY_MAP` in `model/mixin/lineStyle.ts` will miss the property `lineDash`.
This two missing properties can't be fetched by `getItemVisual`, so dash style will be not working in some cases like `lines`,
refer to [chart/helper/Line.ts#L206](https://github.com/apache/incubator-echarts/blob/next/src/chart/helper/Line.ts#L206).
It's different from [line series](https://github.com/apache/incubator-echarts/blob/next/src/chart/line/LineView.ts#L543) and [4.x](https://github.com/apache/incubator-echarts/blob/master/src/chart/helper/Line.js#L374) which are using `lineStyleModel.getLineStyle()`
So how should I do to make it work for those series dependent on `chart/helper/Line.js`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] plainheart commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r470505957
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
That should be feasible and can reduce the options. I'll tweak it later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] echarts-bot[bot] commented on pull request #12961: feat: more configurable styles for lineStyle.
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#issuecomment-657466857
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/incubator-echarts/wiki/How-to-make-a-pull-request).
The pull request is marked to be `PR: author is committer` because you are a committer of this project.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r471086187
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
`style` in `getVisual` is encoded in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L48.
Perhaps we can also add `['lineDash', 'borderType']` in the `ITEM_STYLE_KEY_MAP` and `LINE_STYLE_KEY_MAP`.
Also, your comment reminds me of why the `borderType` in the next branch not work. We need to normalize the style returned by the `styleMapper` in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L77
For example
```ts
// In the normalizeLineDash, it will convert lineDash: 'dashed', 'dotted' to [5, 5], [1, 1].
normalizeLineDash(globalStyle);
```
> We added a semantic value bolder in #13013, but I found this special value will affect the calculation of default dash/dot size and return an illegal value NaN. Perhaps, we should change the logic about either LineView or model/mixin/lineStyle?
Combined with this issue, I think I will prefer to do the normalization of `lineDash` in zrender. Both in SVG and Canvas renderer.
https://github.com/ecomfe/zrender/blob/next/src/svg/graphic.ts#L110
https://github.com/ecomfe/zrender/blob/next/src/canvas/graphic.ts#L172
It will make the `lineDash` processing much easier to be consistent in the echarts code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on pull request #12961: feat: more configurable styles for lineStyle.
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#issuecomment-658533867
Looks great!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r471086187
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
`style` in `getVisual` is encoded in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L48.
Perhaps we can also add `['lineDash', 'borderType']` in the `ITEM_STYLE_KEY_MAP` and `LINE_STYLE_KEY_MAP`.
Also your comment reminds me of why the `borderType` in the next branch not worked. We needs some post-processing on the style returned by the `styleMapper` in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L77
For example
```ts
// In the normalizeLineDash, it will convert lineDash: 'dashed', 'dotted' to [5, 5], [1, 1].
normalizeLineDash(globalStyle);
```
> We added a semantic value bolder in #13013, but I found this special value will affect the calculation of default dash/dot size and return an illegal value NaN. Perhaps, we should change the logic about either LineView or model/mixin/lineStyle?
Combined with this issue, I think I will prefer to do the normalization of `lineDash` in zrender. Both in SVG and Canvas renderer.
https://github.com/ecomfe/zrender/blob/next/src/svg/graphic.ts#L110
https://github.com/ecomfe/zrender/blob/next/src/canvas/graphic.ts#L172
It will make the `lineDash` processing much easier to be consistent in the echarts code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] echarts-bot[bot] commented on pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#issuecomment-682330253
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] plainheart commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r470588142
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
The second issue:
if we make `borderDashArray/dashArray(line)` to be a value of `borderType/type(line)`,
the `ITEM_STYLE_KEY_MAP` in `model/mixin/itemStyle.ts` will miss the property `borderDash` and
the `LINE_STYLE_KEY_MAP` in `model/mixin/lineStyle.ts` will miss the property `lineDash`.
This two missing properties can't be get by `getItemVisual`, so dash style will be not working in some cases like `lines`,
refer to [chart/helper/Line.ts#L206](https://github.com/apache/incubator-echarts/blob/next/src/chart/helper/Line.ts#L206).
It's different from [line series](https://github.com/apache/incubator-echarts/blob/next/src/chart/line/LineView.ts#L543) and [4.x](https://github.com/apache/incubator-echarts/blob/master/src/chart/helper/Line.js#L374) which are using `lineStyleModel.getLineStyle()`
So how should I do to make it work for those series dependent on `chart/helper/Line.js`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r471086187
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
`style` in `getVisual` is encoded in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L48.
Perhaps we can also add `['lineDash', 'borderType']` in the `ITEM_STYLE_KEY_MAP` and `LINE_STYLE_KEY_MAP`.
Also your comment reminds me of why the `borderType` in the next branch not worked. We need some normalization on the style returned by the `styleMapper` in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L77
For example
```ts
// In the normalizeLineDash, it will convert lineDash: 'dashed', 'dotted' to [5, 5], [1, 1].
normalizeLineDash(globalStyle);
```
> We added a semantic value bolder in #13013, but I found this special value will affect the calculation of default dash/dot size and return an illegal value NaN. Perhaps, we should change the logic about either LineView or model/mixin/lineStyle?
Combined with this issue, I think I will prefer to do the normalization in zrender. Both in SVG and Canvas renderer.
https://github.com/ecomfe/zrender/blob/next/src/svg/graphic.ts#L110
https://github.com/ecomfe/zrender/blob/next/src/canvas/graphic.ts#L172
It will make the `lineDash` processing much easier to be consistent in the echarts code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r471086187
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
`style` in `getVisual` is encoded in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L48.
Perhaps we can also add `['lineDash', 'borderType']` in the `ITEM_STYLE_KEY_MAP` and `LINE_STYLE_KEY_MAP`.
Also your comment reminds me of why the `borderType` in the next branch not worked. We needs some post-processing on the style returned by the `styleMapper` in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L77
For example
```ts
// In the normalizeLineDash, it will convert lineDash: 'dashed', 'dotted' to [5, 5], [1, 1].
normalizeLineDash(globalStyle);
```
> We added a semantic value bolder in #13013, but I found this special value will affect the calculation of default dash/dot size and return an illegal value NaN. Perhaps, we should change the logic about either LineView or model/mixin/lineStyle?
Combined with this issue, I think I will prefer to do the normalization in zrender. Both in SVG and Canvas renderer.
https://github.com/ecomfe/zrender/blob/next/src/svg/graphic.ts#L110
https://github.com/ecomfe/zrender/blob/next/src/canvas/graphic.ts#L172
It will make the `lineDash` processing much easier to be consistent in the echarts code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r470418947
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
I'm not sure, but since these two options can only have one work, can we merge them into one option?
Like `borderType` can be an array to represent `borderDashArray`:
```ts
borderType: 'solid' | 'dashed' | 'dotted' | number[]
```
Similar question on `lineType` and `textBorderType`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] plainheart commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
plainheart commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r470579708
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
@pissang There is an issue puzzling me. We added a semantic value `bolder` in #13013, but I found this special value will affect the calculation of default dash/dot size and return an illegal value `NaN`. Perhaps, we should change the logic about either `LineView` or `model/mixin/lineStyle`?
https://github.com/apache/incubator-echarts/blob/next/src/chart/line/LineView.ts#L553-L557
https://github.com/apache/incubator-echarts/blob/next/src/model/mixin/lineStyle.ts#L53
https://github.com/apache/incubator-echarts/blob/next/src/model/mixin/lineStyle.ts#L62-L63
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r471086187
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
`style` in `getVisual` is encoded in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L48.
Perhaps we can also add `['lineDash', 'borderType']` in the `ITEM_STYLE_KEY_MAP` and `LINE_STYLE_KEY_MAP`.
Also your comment reminds me of why the `borderType` in the next branch not worked. We needs some normalization on the style returned by the `styleMapper` in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L77
For example
```ts
// In the normalizeLineDash, it will convert lineDash: 'dashed', 'dotted' to [5, 5], [1, 1].
normalizeLineDash(globalStyle);
```
> We added a semantic value bolder in #13013, but I found this special value will affect the calculation of default dash/dot size and return an illegal value NaN. Perhaps, we should change the logic about either LineView or model/mixin/lineStyle?
Combined with this issue, I think I will prefer to do the normalization in zrender. Both in SVG and Canvas renderer.
https://github.com/ecomfe/zrender/blob/next/src/svg/graphic.ts#L110
https://github.com/ecomfe/zrender/blob/next/src/canvas/graphic.ts#L172
It will make the `lineDash` processing much easier to be consistent in the echarts code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r471086187
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
`style` in `getVisual` is encoded in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L48.
Perhaps we can also add `['lineDash', 'borderType']` in the `ITEM_STYLE_KEY_MAP` and `LINE_STYLE_KEY_MAP`.
Also, your comment reminds me of why the `borderType` in the next branch not work. We need some normalization on the style returned by the `styleMapper` in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L77
For example
```ts
// In the normalizeLineDash, it will convert lineDash: 'dashed', 'dotted' to [5, 5], [1, 1].
normalizeLineDash(globalStyle);
```
> We added a semantic value bolder in #13013, but I found this special value will affect the calculation of default dash/dot size and return an illegal value NaN. Perhaps, we should change the logic about either LineView or model/mixin/lineStyle?
Combined with this issue, I think I will prefer to do the normalization of `lineDash` in zrender. Both in SVG and Canvas renderer.
https://github.com/ecomfe/zrender/blob/next/src/svg/graphic.ts#L110
https://github.com/ecomfe/zrender/blob/next/src/canvas/graphic.ts#L172
It will make the `lineDash` processing much easier to be consistent in the echarts code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang merged pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [incubator-echarts] pissang commented on a change in pull request #12961: feat: more configurable styles for lineStyle & borderStyle.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #12961:
URL: https://github.com/apache/incubator-echarts/pull/12961#discussion_r471086187
##########
File path: src/model/mixin/itemStyle.ts
##########
@@ -54,15 +64,24 @@ class ItemStyleMixin {
includes?: readonly (keyof ItemStyleOption)[]
): ItemStyleProps {
const style = getItemStyle(this, excludes, includes);
- const lineDash = this.getBorderLineDash();
- lineDash && (style.lineDash = lineDash);
+ style.lineDash = this.getBorderLineDash(style.lineWidth);
return style;
}
- getBorderLineDash(this: Model): number[] {
+ getBorderLineDash(this: Model, lineWidth?: number): number[] {
const lineType = this.get('borderType');
- return (lineType === 'solid' || lineType == null) ? null
- : (lineType === 'dashed' ? [5, 5] : [1, 1]);
+ if (lineType == null || lineType === 'solid') {
+ return [];
+ }
+
+ lineWidth = lineWidth || 0;
+
+ let dashArray = this.get('borderDashArray');
+ // compatible with single number
+ if (dashArray != null && !isNaN(dashArray)) {
+ dashArray = [+dashArray];
+ }
+ return dashArray || (lineType === 'dashed' ? [5, 5] : [1, 1]);
}
Review comment:
`style` in `getVisual` is encoded in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L48.
Perhaps we can also add `['lineDash', 'borderType']` in the `ITEM_STYLE_KEY_MAP` and `LINE_STYLE_KEY_MAP`.
Also, your comment reminds me of why the `borderType` in the next branch not work. We need some post-processing on the style returned by the `styleMapper` in https://github.com/apache/incubator-echarts/blob/next/src/visual/style.ts#L77
For example
```ts
// In the normalizeLineDash, it will convert lineDash: 'dashed', 'dotted' to [5, 5], [1, 1].
normalizeLineDash(globalStyle);
```
> We added a semantic value bolder in #13013, but I found this special value will affect the calculation of default dash/dot size and return an illegal value NaN. Perhaps, we should change the logic about either LineView or model/mixin/lineStyle?
Combined with this issue, I think I will prefer to do the normalization of `lineDash` in zrender. Both in SVG and Canvas renderer.
https://github.com/ecomfe/zrender/blob/next/src/svg/graphic.ts#L110
https://github.com/ecomfe/zrender/blob/next/src/canvas/graphic.ts#L172
It will make the `lineDash` processing much easier to be consistent in the echarts code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org