100pah commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2084369026
##########
src/chart/pie/PieSeries.ts:
##########
@@ -278,13 +278,13 @@ class PieSeriesModel extends SeriesModel<PieSeriesOption>
{
// 引导线两段中的第一段长度
length: 15,
// 引导线两段中的第二段长度
- length2: 15,
+ length2: 30,
smooth: false,
minTurnAngle: 90,
maxSurfaceAngle: 90,
lineStyle: {
// color: 各异,
- width: 1,
+ width: 2,
Review Comment:
<img width="546" alt="image"
src="https://github.com/user-attachments/assets/f1ea7e27-cd36-4fe1-bcf2-e80a82ac0bd0";
/>
I think it's too thick and over highlight the guide line, and the linecap is
not pretty enough for a thick guildline.
##########
src/visual/tokens.ts:
##########
@@ -169,6 +174,26 @@ extend(color, {
axisMinorSplitLine: color.neutral05,
} as Tokens['color']);
+for (const key in color) {
+ if (color.hasOwnProperty(key)) {
+ const hex = color[key as keyof ColorToken] as string;
+ if (key === 'theme') {
+ // Don't modify theme colors.
+ tokens.darkColor.theme = color.theme.slice();
+ }
+ else if (key.indexOf('accent') === 0) {
+ // Desaturate and lighten accent colors.
+ // @ts-ignore-next-line
Review Comment:
Shouldn't use ts-ignore.
since `modifyHSL` has been modified along with this PR, the incorrect TS
declaration of `modifyHSL` should be corrected as well.
##########
src/chart/gauge/GaugeSeries.ts:
##########
@@ -34,6 +34,7 @@ import {
} from '../../util/types';
import GlobalModel from '../../model/Global';
import SeriesData from '../../data/SeriesData';
+import tokens from '../../visual/tokens';
Review Comment:
The default border and text background are too light:
<img width="1114" alt="image"
src="https://github.com/user-attachments/assets/7ee83aac-c48f-445c-af06-4516bfe8925e";
/>
<img width="405" alt="image"
src="https://github.com/user-attachments/assets/c743c25c-9a5b-4feb-8560-ae2c7a46dd87";
/>
previous:
<img width="1042" alt="image"
src="https://github.com/user-attachments/assets/169a2c33-35bc-45cf-80b4-468a5fb9e500";
/>
##########
src/coord/geo/GeoModel.ts:
##########
@@ -203,20 +204,20 @@ class GeoModel extends ComponentModel<GeoOption> {
emphasis: {
label: {
show: true,
- color: 'rgb(100,0,0)'
+ color: tokens.color.primary
},
itemStyle: {
- color: 'rgba(255,215,0,0.8)'
+ areaColor: tokens.color.background
}
},
select: {
label: {
show: true,
- color: 'rgb(100,0,0)'
+ color: tokens.color.primary
},
itemStyle: {
- color: 'rgba(255,215,0,0.8)'
+ color: tokens.color.backgroundShade
Review Comment:
The selected style should be reverted, like map issue. We should not use a
grayscale to represent selected. It's hard to distinguish from "selected" and
"unselected" area, and a grayscale commonly indicate disable.
##########
src/component/legend/ScrollableLegendModel.ts:
##########
@@ -95,11 +96,11 @@ class ScrollableLegendModel extends
LegendModel<ScrollableLegendOption> {
horizontal: ['M0,0L12,-10L12,10z', 'M0,0L-12,-10L-12,10z'],
vertical: ['M0,0L20,0L10,-20z', 'M0,0L20,0L10,20z']
},
- pageIconColor: '#2f4554',
- pageIconInactiveColor: '#aaa',
+ pageIconColor: tokens.color.tertiary,
+ pageIconInactiveColor: tokens.color.disabled,
pageIconSize: 15, // Can be [10, 3], which represents [width, height]
pageTextStyle: {
- color: '#333'
+ color: tokens.color.tertiary
Review Comment:
Grayscale color in pager button and pager texts commonly indicate disabled.
Could it be more dark to avoid misleading?
<img width="138" alt="image"
src="https://github.com/user-attachments/assets/8eca73c0-511b-46c0-b62c-c351ceb715bd";
/>
##########
src/theme/light.ts:
##########
@@ -1,35 +0,0 @@
-/*
Review Comment:
Should not remove this file. Rename and keep it, thus users need this theme
can still use it.
##########
src/chart/scatter/ScatterSeries.ts:
##########
@@ -151,7 +152,7 @@ class ScatterSeriesModel extends
SeriesModel<ScatterSeriesOption> {
select: {
itemStyle: {
- borderColor: '#212121'
+ borderColor: tokens.color.primary
Review Comment:
The default select style seems hard to tell:
<img width="915" alt="image"
src="https://github.com/user-attachments/assets/8422983e-cfc1-47d0-9f33-a097d2f87ae7";
/>
(tho previously they are also hard to tell)
I've no idea whether we need to give a more noticeable select style, like
thicker borderWidth.
But it's OK to keep it as is if too complicated to set them everywhere.
##########
src/component/tooltip/TooltipModel.ts:
##########
@@ -137,6 +138,7 @@ class TooltipModel extends ComponentModel<TooltipOption> {
// tooltip border width, unit is px, default is 0 (no border)
borderWidth: 1,
+ borderColor: tokens.color.borderShade,
Review Comment:
Previously the tooltip border color is responsive to the current focused
series:
<img width="562" alt="image"
src="https://github.com/user-attachments/assets/574821a4-2ef0-421b-a54a-c33f16522477";
/>
<img width="473" alt="image"
src="https://github.com/user-attachments/assets/b1596e56-fc5d-42a2-8cc6-01bfd4c634b3";
/>
This feature may help users to distinguish the targeted item when numerous
items are close.
The new style remove this feature and use a gray. Is it deliberately removed
or a mistake?
I personally prefer the previous feature.
<img width="819" alt="image"
src="https://github.com/user-attachments/assets/52041d80-9cdf-4059-bb7f-f866a32b264e";
/>
##########
src/core/echarts.ts:
##########
@@ -3231,7 +3230,7 @@ function makeSelectChangedEvent(
}
// Default theme
-registerTheme('light', lightTheme);
+registerTheme('light', {});
Review Comment:
I think we can remove this line. `registerTheme('light', {})`.
##########
src/coord/cartesian/GridModel.ts:
##########
@@ -52,17 +53,17 @@ class GridModel extends ComponentModel<GridOption>
implements CoordinateSystemHo
show: false,
// zlevel: 0,
z: 0,
- left: '10%',
- top: 60,
- right: '10%',
- bottom: 70,
+ left: tokens.size.xl,
+ top: 65,
+ right: tokens.size.xl,
+ bottom: 60,
// If grid size contain label
- containLabel: false,
+ containLabel: true,
Review Comment:
That is a huge breaking change.
I think it should not be `true` by default, because we have set it `false`
for years and a large number of charts are layout based on it. Make that be
breaking brings less benefit but probably bother existing users by breaking the
existing layout.
And when there are multiple charts in one page (quite common in Management
Information System), axes between different charts are usually needed to be
aligned, which requires `containLabel: false`
And after set it back to `false`, we should go back to the original left
right setting `'10%'`
##########
src/component/toolbox/ToolboxModel.ts:
##########
@@ -120,27 +121,27 @@ class ToolboxModel extends ComponentModel<ToolboxOption> {
backgroundColor: 'transparent',
- borderColor: '#ccc',
+ borderColor: tokens.color.border,
borderRadius: 0,
borderWidth: 0,
- padding: 5,
+ padding: tokens.size.m,
itemSize: 15,
- itemGap: 8,
+ itemGap: tokens.size.s,
showTitle: true,
iconStyle: {
- borderColor: '#666',
+ borderColor: tokens.color.accent50,
Review Comment:
About the toolbox style:
1. The new color of the toolbox seems too light, making it less clear or
sharp for the toolbox icon, since they are composed of thin lines.
2. I think the default toolbox style should not changed to a saturated or
chromatic color, since they are auxiliary tool and usually placed on the
top-right (one of the most noticeable place), therefore they should be neutral
and avoid the colors of real series/chart, be more close to grayscale color.
-- new style --
<img width="907" alt="image"
src="https://github.com/user-attachments/assets/ffae31d3-f911-4694-a25e-31c5ef1bd5ff";
/>
-- previous style --
<img width="921" alt="image"
src="https://github.com/user-attachments/assets/00472819-d1a1-4492-813c-2b79de1f65f9";
/>
##########
src/coord/calendar/CalendarModel.ts:
##########
@@ -36,6 +36,7 @@ import {
} from '../../util/types';
import GlobalModel from '../../model/Global';
import Model from '../../model/Model';
+import tokens from '../../visual/tokens';
Review Comment:
I think the default year should not be that dark since it has been a large
text, but the month and day label should be dark because they are more
important ticks than year (they are like axis ticks in cartesian).
-- new style ---
<img width="834" alt="image"
src="https://github.com/user-attachments/assets/a9ce51b3-62de-441c-998a-4c5da93e08d1";
/>
-- previous style ---
<img width="1167" alt="image"
src="https://github.com/user-attachments/assets/2b0a264a-6395-4038-910c-193d328e5fbf";
/>
-- github contribution --
<img width="535" alt="image"
src="https://github.com/user-attachments/assets/e269f39d-3a70-4c3f-b2da-0f2be8f73266";
/>
And a light month and day label will bring that bad case in dark background:
-- new style --
<img width="669" alt="image"
src="https://github.com/user-attachments/assets/e3e931b9-910e-4476-98dd-09e2c54e30a5";
/>
-- previous style --
<img width="662" alt="image"
src="https://github.com/user-attachments/assets/ff7c2431-e358-4db7-8289-8d3a60975fb2";
/>
##########
src/component/timeline/SliderTimelineModel.ts:
##########
@@ -99,47 +100,42 @@ class SliderTimelineModel extends TimelineModel {
position: 'left', // 'left' 'right' 'top' 'bottom'
Review Comment:
I think the new timeline style is less clear than the previous one:
The style of "hover" and "selected" and "current one" are less clear to
distinguish from the normal style.
-- new style -- (hovering on the "play" (leftmost) button):
<img width="441" alt="image"
src="https://github.com/user-attachments/assets/f2139788-97fa-4643-8142-198d4457593c";
/>
-- previous style -- (hovering on the "play" (leftmost) button):
<img width="534" alt="image"
src="https://github.com/user-attachments/assets/09651581-a4ee-4421-a1bd-0c2148603151";
/>
I think the "downplay" style can be changed like the new one, but the other
style should not be changed.
##########
src/chart/funnel/FunnelSeries.ts:
##########
@@ -177,12 +178,12 @@ class FunnelSeriesModel extends
SeriesModel<FunnelSeriesOption> {
length: 20,
lineStyle: {
// color: 各异,
- width: 1
+ width: 2
Review Comment:
The same issue with pie (tho less obvious then pie)
##########
src/component/axisPointer/AxisPointerModel.ts:
##########
@@ -132,11 +133,7 @@ class AxisPointerModel extends
ComponentModel<AxisPointerOption> {
margin: 50,
// color: '#1b8bbd'
// color: '#2f4554'
- color: '#333',
- shadowBlur: 3,
- shadowColor: '#aaa',
- shadowOffsetX: 0,
- shadowOffsetY: 2,
+ color: tokens.color.quaternary,
Review Comment:
<img width="235" alt="image"
src="https://github.com/user-attachments/assets/15f49695-7847-476e-b4ed-068d2ef46b80";
/>
Grey commonly implies disabled. I thinks it is not preferable to be used on
a non-disabled handle. Could it be more dark?
##########
src/chart/line/LineSeries.ts:
##########
@@ -166,7 +167,7 @@ class LineSeriesModel extends SeriesModel<LineSeriesOption>
{
},
lineStyle: {
- width: 2,
+ width: 3,
Review Comment:
I'm afraid the default line width is too thick, make the appearance less
clear and too aggressive - **especially in large canvas area, or a large number
of data, or numerous series, or no symbol (bad linecap effect is amplified)**,
and it seems not common to use that thick line in line chart.
As the most commonly used chart type, I think the default style should be
more neutral or conservative. The default line with should still be `2`.
In current PR the default line width is `3`,
while in v5 it is `2`.
--line width 3--
<img width="553" alt="image"
src="https://github.com/user-attachments/assets/178d8826-2744-4499-bc62-2766321e04a8";
/>
--line width 2--
<img width="540" alt="image"
src="https://github.com/user-attachments/assets/f24e749b-c5a9-485f-bf81-dffedaa42ef6";
/>
---
--line width 3--
<img width="543" alt="image"
src="https://github.com/user-attachments/assets/7b298341-c704-4f72-a6a6-8572fef17b14";
/>
--line width 2--
<img width="517" alt="image"
src="https://github.com/user-attachments/assets/d5427d43-a847-49b6-a633-cfdf922623f1";
/>
---
--line width 3--
<img width="811" alt="image"
src="https://github.com/user-attachments/assets/de5577ee-b68f-4845-85e0-a6c05a46c4a8";
/>
--line width 2--
<img width="816" alt="image"
src="https://github.com/user-attachments/assets/45a8a68d-f154-4259-94ec-6ef305272648";
/>
---
--line width 3--
<img width="925" alt="image"
src="https://github.com/user-attachments/assets/5b0df2e4-ecbd-4c25-93d4-f63d695bb188";
/>
--line width 2--
<img width="925" alt="image"
src="https://github.com/user-attachments/assets/d5fb2782-0724-4297-96a7-f790218c3719";
/>
-- line cap bad case is amplified by line width 3 --
<img width="105" alt="image"
src="https://github.com/user-attachments/assets/0d59bae6-9a20-4cb3-81dc-b96b3e66702d";
/>
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
