You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by "Ovilia (via GitHub)" <gi...@apache.org> on 2025/03/26 11:06:41 UTC
[PR] feat: new theme for ECharts 6.0 [echarts]
Ovilia opened a new pull request, #20865:
URL: https://github.com/apache/echarts/pull/20865
<!-- 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 ONE SENTENCE TO DESCRIBE WHAT THIS PR DOES. -->
### Fixed issues
<!--
- #xxxx: ...
-->
## Details
### Before: What was the problem?
<!-- DESCRIBE THE BUG OR REQUIREMENT HERE. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
### After: How does it behave after the fixing?
<!-- THE RESULT AFTER FIXING AND A SIMPLE EXPLANATION ABOUT HOW IT IS FIXED. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
## Document Info
One of the following should be checked.
- [ ] This PR doesn't relate to document changes
- [ ] The document should be updated later
- [ ] The document changes have been made in apache/echarts-doc#xxx
## Misc
### ZRender Changes
- [ ] This PR depends on ZRender changes (ecomfe/zrender#xxx).
### Related test cases or examples to use the new APIs
N.A.
## Others
### Merging options
- [ ] Please squash the commits into a single one when merging.
### 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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2103951885
##########
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:
Changed to:
<img width="122" alt="image" src="https://github.com/user-attachments/assets/47b49501-8fd7-48a8-804f-070401b8b5d7" />
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2097115982
##########
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:
Upated to a darker gray, not exactly the same as v5 because we changed a bit about the neutral colors in v6.
<img width="933" alt="image" src="https://github.com/user-attachments/assets/75aebcb5-da8c-48a5-bd0c-65ce86f2df93" />
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat(theme): new theme for ECharts 6.0 [echarts]
Posted by "nook24 (via GitHub)" <gi...@apache.org>.
nook24 commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-3739135791
Maybe a rookie question, but is there an option to get the legend back to the top?
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-2853592440
@100pah Thanks for the detailed review!
## Backward Compatibility
Since we aim to reduce the overlapping issues among default components, we've had to adjust the default positions and alignment methods of the components. This might cause some situations where users' original settings lead to overlapping under the new layout, just as in the example you mentioned. However, this is **a carefully considered trade-off**. For instance, I believe the probability of displaying both the legend and the title simultaneously is much higher than that of displaying both the legend and the dataZoom. Suppose the new solution can address 60% of the previous bad-case scenarios while introducing 10% of new ones. Even so, this optimization is still highly valuable.
To completely solve this problem, we would need to come up with **a robust component anti-overlapping mechanism**. Unfortunately, we're likely to run out of time to implement this in 6.0. Therefore, when the time comes, we will provide an **migration document** to inform users about which default values we've modified and how they can adjust the positions in case of overlapping.
## Theme Colors
After much careful consideration and deliberation, I truly believe that this new theme is the optimal one I can come up with. When it comes to aesthetics, it might be rather challenging for me to provide objective evidence. However, in my sincere opinion, this is the best design I could come up with. Designing the theme color requires a comprehensive and holistic consideration. It's not simply about having individual colors that look good on their own; they must also harmonize well when put together.
Therefore, **with due respect to all viewpoints, I'm inclined to uphold this theme**. I understand that different people may have diverse perspectives on color aesthetics, but based on my in-depth consideration and analysis, I believe this theme has its unique merits and can best meet our overall design needs.
I'm more than willing to further discuss and listen to your thoughts to reach a more satisfactory solution together. If any of you have some better ideas regarding the theme color, I would humbly suggest that you could use the [theme builder](https://echarts.apache.org/zh/theme-builder.html) to create a theme think is better. Subsequently, we could further discuss it through a community voting process.
As about the issue that the third color is relatively close to a neutral color, I believe this is a design choice after comprehensively considering various factors. The reasons for this design were also stated in the PR description. If you have any objections, I would suggest you propose a better overall design plan. This PR currently doesn't have the logic for a dark mode. Whether certain colors need to be adjusted may be further discussed in the PR for the dark mode in the future. My preliminary plan is to calculate the colors for the dark mode dynamically based on the light mode. If some colors need adjustment, we can then consider whether corresponding methods need to be provided.
Regarding the concern that the green might be too light and the contrast for labels is insufficient, this issue isn't actually caused by the selection of the theme color. Instead, it stems from the inadequate optimization of label colors for some series. For example, pie and funnel both use label/labelLine design, why is the label color of the pie chart dark gray, while the funnel chart's label takes the theme color? Considering design consistency, could we **change the default label color of the funnel chart to the same dark gray as that of the pie chart**?
In the Treemap, the yellow in the original theme color also has a problem with insufficient brightness contrast between the background color and the label color. Similar to how the [bar chart automatically calculates whether to use white or dark gray for the label](https://echarts.apache.org/examples/zh/editor.html?c=bar-label-rotation) based on the brightness of the theme color when the label is inside, wouldn't it be more reasonable if we adopted the same approach for the Treemap?
## Highlight/emphasis color of map
If you think the current slightly darker gray isn't eye-catching enough, I think we could change it to light blue. However, I'm concerned that this color might cause confusion when using visualMap. Could a darker gray be acceptable to you? The originally yellow and red is never used elsewhere and the color choice is not elegant, so I feel necessary to change them.
I don't quite agree with the statement "since users typically use map individually and do not combinate maps with other types of chart". Maps are likely to appear in the same dashboard as other charts, not to mention that we even support designs like displaying pie charts on the map coordinate.
## Visual map color
Using darker color for smaller value was a mistake. I didn't check carefully before and will fix this later.
Using a mono-tone color in visual mapping could be a better design choice.
Firstly, a mono-tone design is simpler and more likely to achieve harmony. This might well be the reason why GitHub adopts a mono-tone green design.
Secondly, the introduction of red may imply dangerous or negative connotations. However, in many cases, a large value doesn't necessarily carry such meanings. Therefore, for the default design, a more neutral semantic expression is, in my opinion, more appropriate.
Thirdly, in the gradient from red to blue as you suggested, there is a light-blue to dark-blue section. In fact, the value corresponding to light blue is larger than that of dark blue, just as you pointed out above, dark colors usually represent larger values. Thus, using a mono-tone color can reduce such misunderstandings.
## axisTick
As we can see from the following demos, most chart libraries use no axis tick for default.
I believe again, this is a design choice. The benefit of having a neat design by not showing ticks overweighs the disadvantage of data grouping. Maybe we can use a smaller `barGap` by default to better indicate the grouping.
Ticks should not be hidden for time series. Can you provide the option for your time axis for me to check?
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2097117516
##########
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:
Reverted.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2097173317
##########
src/chart/line/LineSeries.ts:
##########
@@ -166,7 +167,7 @@ class LineSeriesModel extends SeriesModel<LineSeriesOption> {
},
lineStyle: {
- width: 2,
+ width: 3,
Review Comment:
Reverted.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2097288192
##########
src/core/echarts.ts:
##########
@@ -3231,7 +3230,7 @@ function makeSelectChangedEvent(
}
// Default theme
-registerTheme('light', lightTheme);
+registerTheme('light', {});
Review Comment:
Changed to `registerTheme('default', {})` so that we can use `chart.setTheme('default')` to revert to the default theme after changing to other themes.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "diysimon (via GitHub)" <gi...@apache.org>.
diysimon commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-2830121276
Very professional design, looking forward to the official release of 6.0！
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat(theme): new theme for ECharts 6.0 [echarts]
Posted by "Russiarain (via GitHub)" <gi...@apache.org>.
Russiarain commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-3741033400
> Maybe a rookie question, but is there an option to get the legend back to the top?
Just add a `top` property to legend.
```json
legend: {
top: 'top',
}
```
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-2754048281
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-20865@4c9428b
<!-- ECHARTS_PR_PREVIEW -->
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Russiarain (via GitHub)" <gi...@apache.org>.
Russiarain commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-2833700987
Great design! Just one question, will there be enough contrast to the surface(maybe `#1e1e1e`) in dark mode for the third theme color?
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
100pah commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2085047661
##########
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%'`
Regarding what's the ideal setting of `containLabel`:
- `containLabel: false` may cause long labels out of viewport
- `containLabel: true` loose the ability to align axis line between different charts.
I'm trying to implement a feature that reconcile them, that is, `left/right/top/bottom/width/height` indicate a rect for axis lines (in `containLabel: false` way), but if the labels are out of viewport, shrink the rect to avoid it.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2097128091
##########
src/chart/scatter/ScatterSeries.ts:
##########
@@ -151,7 +152,7 @@ class ScatterSeriesModel extends SeriesModel<ScatterSeriesOption> {
select: {
itemStyle: {
- borderColor: '#212121'
+ borderColor: tokens.color.primary
Review Comment:
Changed borderWdith to be 2.
<img width="479" alt="image" src="https://github.com/user-attachments/assets/3aadaf8b-a41b-4769-a041-cf7cefaee026" />
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2085822728
##########
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:
Yes, this was a bug introduced when I tried to add a tooltip border to dark mode. I'll check this 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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat(theme): new theme for ECharts 6.0 [echarts]
Posted by "Justineo (via GitHub)" <gi...@apache.org>.
Justineo commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-3022193691
From my personal point of view, the new color scheme doesn’t feel quite as visually appealing as the previous version. I’m wondering if we might have placed a stronger emphasis on accessibility this time? I’d love to better understand the guiding principles behind how we define the primary color palette — for instance, how accessibility considerations are prioritized in the overall design process. Do we typically validate the results through steps like color vision deficiency simulation or similar regression testing?
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "echarts-bot[bot] (via GitHub)" <gi...@apache.org>.
echarts-bot[bot] commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-2754033156
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/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.
⚠️ MISSING DOCUMENT INFO: Please make sure one of the document options are checked in this PR's description. Search "Document Info" in the description of this PR. This should be done either by the author or the reviewers of the PR.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-2862806830
Updated funnel label style to be compat with pie.
<img width="514" alt="image" src="https://github.com/user-attachments/assets/d7f33d7c-0f37-4b67-aaeb-0cdc87957067" />
Changed map emphasis color to be yellow.
<img width="509" alt="image" src="https://github.com/user-attachments/assets/922bf7ab-7842-4570-879f-09863710a22e" />
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2097276668
##########
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:
Changed the year label to be a lighter gray, but not as light as v5. I don't quite believe we should make it lighter since it's larger. It's larger because it's more prominant, so it gives more reason to make it darker.
<img width="1150" alt="image" src="https://github.com/user-attachments/assets/8920f0e2-1106-4839-846d-0e1393cd25e5" />
Dark background should not be considered in this case. If user wants to use dark background, he, or the one who makes the dark theme should be responsible for changing the label colors to fit the background. And whatever color you choose for the default label, it's still possible that the user set the background to a similar color (a black background for example).
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2103964086
##########
src/component/timeline/SliderTimelineModel.ts:
##########
@@ -99,47 +100,42 @@ class SliderTimelineModel extends TimelineModel {
position: 'left', // 'left' 'right' 'top' 'bottom'
Review Comment:
I prefer not to use the shadow because there are no other components using such design language. Changed to:
<img width="290" alt="image" src="https://github.com/user-attachments/assets/dedf2959-2723-44b3-b91e-e060cc1c5d1b" />
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "plainheart (via GitHub)" <gi...@apache.org>.
plainheart commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2125774374
##########
src/component/tooltip/TooltipView.ts:
##########
@@ -57,6 +57,7 @@ import { normalizeTooltipFormatResult } from '../../model/mixin/dataFormat';
import { createTooltipMarkup, buildTooltipMarkup, TooltipMarkupStyleCreator } from './tooltipMarkup';
import { findEventDispatcher } from '../../util/event';
import { clear, createOrUpdate } from '../../util/throttle';
+import tokens from '../../visual/tokens';
Review Comment:
Remove the unused `tokens` import to fix this lint warning.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
100pah commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2085047661
##########
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%'`
Regarding what's the ideal setting of `containLabel`:
- `containLabel: false` may cause long labels out of viewport
- `containLabel: true` loose the ability to align axis lines between different charts.
I'm trying to implement a feature that reconcile them, that is, `left/right/top/bottom/width/height` indicate a rect for axis lines (in `containLabel: false` way), but if the labels are out of viewport, shrink the rect to avoid it.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2097117867
##########
src/chart/funnel/FunnelSeries.ts:
##########
@@ -177,12 +178,12 @@ class FunnelSeriesModel extends SeriesModel<FunnelSeriesOption> {
length: 20,
lineStyle: {
// color: 各异,
- width: 1
+ width: 2
Review Comment:
Reverted.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2097117123
##########
src/theme/light.ts:
##########
@@ -1,35 +0,0 @@
-/*
Review Comment:
Moved to `/theme/rainbow.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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
100pah commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2118055721
##########
test/runTest/client/client.js:
##########
@@ -156,17 +197,34 @@ const app = new Vue({
runConfig: Object.assign({
sortBy: 'name',
- actualVersion: null,
+ actualVersion: 'local',
Review Comment:
I think this modification is not correct.
After this change, the initial state will be:
<img width="657" alt="image" src="https://github.com/user-attachments/assets/3961dc29-899a-464b-b998-b9987800eaa1" />
The actual "Release" is not supposed to have a "local" branch.
I used to fix that issue in 365f33be8982ce1055dac5ffa40fdf6c03bd489d , but this change it back. Is it intentional?
I think in most cases the "actual" should be local, rather than release.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
100pah commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2104201888
##########
src/component/timeline/SliderTimelineModel.ts:
##########
@@ -99,47 +100,42 @@ class SliderTimelineModel extends TimelineModel {
position: 'left', // 'left' 'right' 'top' 'bottom'
- playIcon: 'path://M31.6,53C17.5,53,6,41.5,6,27.4S17.5,1.8,31.6,1.8C45.7,1.8,57.2,13.3,57.2,27.4S45.7,53,31.6,53z M31.6,3.3 C18.4,3.3,7.5,14.1,7.5,27.4c0,13.3,10.8,24.1,24.1,24.1C44.9,51.5,55.7,40.7,55.7,27.4C55.7,14.1,44.9,3.3,31.6,3.3z M24.9,21.3 c0-2.2,1.6-3.1,3.5-2l10.5,6.1c1.899,1.1,1.899,2.9,0,4l-10.5,6.1c-1.9,1.1-3.5,0.2-3.5-2V21.3z', // jshint ignore:line
- stopIcon: 'path://M30.9,53.2C16.8,53.2,5.3,41.7,5.3,27.6S16.8,2,30.9,2C45,2,56.4,13.5,56.4,27.6S45,53.2,30.9,53.2z M30.9,3.5C17.6,3.5,6.8,14.4,6.8,27.6c0,13.3,10.8,24.1,24.101,24.1C44.2,51.7,55,40.9,55,27.6C54.9,14.4,44.1,3.5,30.9,3.5z M36.9,35.8c0,0.601-0.4,1-0.9,1h-1.3c-0.5,0-0.9-0.399-0.9-1V19.5c0-0.6,0.4-1,0.9-1H36c0.5,0,0.9,0.4,0.9,1V35.8z M27.8,35.8 c0,0.601-0.4,1-0.9,1h-1.3c-0.5,0-0.9-0.399-0.9-1V19.5c0-0.6,0.4-1,0.9-1H27c0.5,0,0.9,0.4,0.9,1L27.8,35.8L27.8,35.8z', // jshint ignore:line
+ playIcon: 'path://M15 0C23.2843 0 30 6.71573 30 15C30 23.2843 23.2843 30 15 30C6.71573 30 0 23.2843 0 15C0 6.71573 6.71573 0 15 0ZM15 3C8.37258 3 3 8.37258 3 15C3 21.6274 8.37258 27 15 27C21.6274 27 27 21.6274 27 15C27 8.37258 21.6274 3 15 3ZM11.5 10.6699C11.5 9.90014 12.3333 9.41887 13 9.80371L20.5 14.1338C21.1667 14.5187 21.1667 15.4813 20.5 15.8662L13 20.1963C12.3333 20.5811 11.5 20.0999 11.5 19.3301V10.6699Z', // jshint ignore:line
+ stopIcon: 'path://M15 0C23.2843 0 30 6.71573 30 15C30 23.2843 23.2843 30 15 30C6.71573 30 0 23.2843 0 15C0 6.71573 6.71573 0 15 0ZM15 3C8.37258 3 3 8.37258 3 15C3 21.6274 8.37258 27 15 27C21.6274 27 27 21.6274 27 15C27 8.37258 21.6274 3 15 3ZM11.5 10C12.3284 10 13 10.6716 13 11.5V18.5C13 19.3284 12.3284 20 11.5 20C10.6716 20 10 19.3284 10 18.5V11.5C10 10.6716 10.6716 10 11.5 10ZM18.5 10C19.3284 10 20 10.6716 20 11.5V18.5C20 19.3284 19.3284 20 18.5 20C17.6716 20 17 19.3284 17 18.5V11.5C17 10.6716 17.6716 10 18.5 10Z', // jshint ignore:line
// eslint-disable-next-line max-len
- nextIcon: 'M2,18.5A1.52,1.52,0,0,1,.92,18a1.49,1.49,0,0,1,0-2.12L7.81,9.36,1,3.11A1.5,1.5,0,1,1,3,.89l8,7.34a1.48,1.48,0,0,1,.49,1.09,1.51,1.51,0,0,1-.46,1.1L3,18.08A1.5,1.5,0,0,1,2,18.5Z', // jshint ignore:line
+ nextIcon: 'path://M0.838834 18.7383C0.253048 18.1525 0.253048 17.2028 0.838834 16.617L7.55635 9.89949L0.838834 3.18198C0.253048 2.59619 0.253048 1.64645 0.838834 1.06066C1.42462 0.474874 2.37437 0.474874 2.96015 1.06066L10.7383 8.83883L10.8412 8.95277C11.2897 9.50267 11.2897 10.2963 10.8412 10.8462L10.7383 10.9602L2.96015 18.7383C2.37437 19.3241 1.42462 19.3241 0.838834 18.7383Z', // jshint ignore:line
// eslint-disable-next-line max-len
- prevIcon: 'M10,.5A1.52,1.52,0,0,1,11.08,1a1.49,1.49,0,0,1,0,2.12L4.19,9.64,11,15.89a1.5,1.5,0,1,1-2,2.22L1,10.77A1.48,1.48,0,0,1,.5,9.68,1.51,1.51,0,0,1,1,8.58L9,.92A1.5,1.5,0,0,1,10,.5Z', // jshint ignore:line
+ prevIcon: 'path://M10.9602 1.06066C11.5459 1.64645 11.5459 2.59619 10.9602 3.18198L4.24264 9.89949L10.9602 16.617C11.5459 17.2028 11.5459 18.1525 10.9602 18.7383C10.3744 19.3241 9.42462 19.3241 8.83883 18.7383L1.06066 10.9602L0.957771 10.8462C0.509245 10.2963 0.509245 9.50267 0.957771 8.95277L1.06066 8.83883L8.83883 1.06066C9.42462 0.474874 10.3744 0.474874 10.9602 1.06066Z', // jshint ignore:line
prevBtnSize: 18,
nextBtnSize: 18,
- color: '#A4B1D7',
- borderColor: '#A4B1D7',
- borderWidth: 1
+ color: tokens.color.accent50,
+ borderColor: tokens.color.accent50,
+ borderWidth: 0
},
emphasis: {
label: {
show: true,
// 其余属性默认使用全局文本样式，详见TEXTSTYLE
- color: '#6f778d'
+ color: tokens.color.accent60
},
itemStyle: {
- color: '#316BF3'
+ color: tokens.color.accent60
},
controlStyle: {
- color: '#316BF3',
- borderColor: '#316BF3',
- borderWidth: 2
- }
+ color: tokens.color.accent70
Review Comment:
I think here it should be :
```ts
controlStyle: {
color: tokens.color.accent70,
borderColor: tokens.color.accent70,
},
```
Consider users set
```ts
controlStyle: {
emphasis: {
borderWidth: 1
}
},
```
normal style:
<img width="48" alt="image" src="https://github.com/user-attachments/assets/8a65dc5c-024d-4a43-944a-cbde2de6033d" />
emphasis style in this PR (a little bit weird):
<img width="44" alt="image" src="https://github.com/user-attachments/assets/a90c982e-783c-40b9-b1e9-13b014a765f0" />
emphasis style expect:
<img width="52" alt="image" src="https://github.com/user-attachments/assets/b6f050a9-7a5f-4471-8d90-7d768fc38681" />
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2103926052
##########
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:
Changed to:
<img width="245" alt="image" src="https://github.com/user-attachments/assets/2191bc83-f27e-4133-a0d4-5b0a0fdafb74" />
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-2861723772
In the above commit, I added a new option `grid.containDataZoom`. Similar to `containLabel`, it adjust grid position to make room for dataZoom. @100pah Do you feel this is the correct way to fix the problem?
## v5
<img width="1046" alt="image" src="https://github.com/user-attachments/assets/dd6bb5fe-aeed-424c-86e2-a05ea8b8c679" />
## v6
<img width="1046" alt="image" src="https://github.com/user-attachments/assets/c46df2b2-c71f-4200-b450-433670d60685" />
Option is:
```js
{
show: true,
realtime: true,
start: 30,
end: 70
},
{
show: true,
realtime: true,
yAxisIndex: 0
}
```
and
```js
{
show: true,
realtime: true,
start: 30,
end: 70,
top: 50
},
{
show: true,
realtime: true,
yAxisIndex: 0,
left: 25
}
```
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
100pah commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2085047661
##########
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%'`
Regarding what's the ideal setting of `containLabel`:
- `containLabel: false` may cause long labels out of viewport
- `containLabel: true` loose the ability to align axis line between different charts.
I'm trying to implement a feature that reconcile them, that is, `left/right/top/bottom/width/height` indicate a rect for axis lines (in `containLabel: false` way), but if the labels out of viewport, shrink the rect to avoid it.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2103930449
##########
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:
Changed to:
<img width="86" alt="image" src="https://github.com/user-attachments/assets/b83424f1-efc6-4570-9597-e8e59ef1729b" />
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "echarts-bot[bot] (via GitHub)" <gi...@apache.org>.
echarts-bot[bot] commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-2942899167
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.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "Ovilia (via GitHub)" <gi...@apache.org>.
Ovilia merged PR #20865:
URL: https://github.com/apache/echarts/pull/20865
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
100pah commented on code in PR #20865:
URL: https://github.com/apache/echarts/pull/20865#discussion_r2061397276
##########
src/visual/tokens.ts:
##########
@@ -0,0 +1,116 @@
+const tokens = {
+ color: {
+ theme: [
+ '#5978e3',
+ '#b6d634',
+ '#505372',
+ '#ff994d',
+ '#0ca8df',
+ '#ffd10a',
+ '#fb628b',
+ '#785db0',
+ '#3fbe95'
+ ],
+
+ neutral00: '#fff',
+ neutral05: '#f4f7fd',
+ neutral10: '#e8ebf0',
+ neutral15: '#dbdee4',
+ neutral20: '#cfd2d7',
+ neutral25: '#c3c5cb',
+ neutral30: '#b7b9be',
+ neutral35: '#aaacb2',
+ neutral40: '#9ea0a5',
+ neutral45: '#929399',
+ neutral50: '#86878c',
+ neutral55: '#797b7f',
+ neutral60: '#6d6e73',
+ neutral65: '#616266',
+ neutral70: '#54555a',
+ neutral75: '#48494d',
+ neutral80: '#3c3c41',
+ neutral85: '#303034',
+ neutral90: '#232328',
+ neutral95: '#17171b',
+ neutral99: '#000',
+
+ accent05: '#eef2fc',
+ accent10: '#dee4f9',
+ accent15: '#cdd7f7',
+ accent20: '#bdc9f4',
+ accent25: '#acbcf1',
+ accent30: '#9baeee',
+ accent35: '#8ba1eb',
+ accent40: '#7a93e9',
+ accent45: '#6a86e6',
+ accent50: '#5978e3',
+ accent55: '#506ccc',
+ accent60: '#4760b6',
+ accent65: '#3e549f',
+ accent70: '#354888',
+ accent75: '#2d3c72',
+ accent80: '#24305b',
+ accent85: '#1b2444',
+ accent90: '#12182d',
+ accent95: '#090c17',
+
+ transparent: 'rgba(0,0,0,0)',
+
+ primary: '@neutral80',
+ secondary: '@neutral70',
+ tertiary: '@neutral60',
+ quaternary: '@neutral50',
+ disabled: '@neutral20',
+
+ border: '@neutral30',
+ borderTint: '@neutral20',
+ borderShade: '@neutral40',
+
+ background: '@neutral05',
+ backgroundTint: 'rgba(234,237,245,0.5)',
+ backgroundTransparent: 'rgba(255,255,255,0)',
+ backgroundShade: '@neutral10',
+
+ shadow: 'rgba(0,0,0,0.2)',
+ shadowTint: 'rgba(129,130,136,0.2)',
+
+ axisLine: '@neutral70',
+ axisLineTint: '@neutral40',
+ axisTick: '@axisLine',
+ axisTickMinor: '@neutral60',
+ axisLabel: '@axisLine',
+ axisSplitLine: '@neutral15',
+ axisMinorSplitLine: '@neutral05'
+ },
+ size: {
+ xxs: 2,
+ xs: 5,
+ s: 10,
+ m: 15,
+ l: 20,
+ xl: 30,
+ xxl: 40,
+ xxxl: 50
+ }
+} as const;
+
+function decodeTokens() {
+ const color = tokens.color as {
+ [key: string]: string | readonly string[]
+ };
+ for (const key in color) {
+ if (Object.prototype.hasOwnProperty.call(color, key)) {
+ const value = color[key];
+ if (typeof value === 'string' && value.startsWith('@')) {
+ const refKey = value.slice(1);
+ if (refKey in color) {
+ color[key] = color[refKey];
+ }
+ }
+ }
+ }
+}
Review Comment:
I think this decodeToken and "@" grammar is not correct enough as a common util; and not necessary as local function.
### Not correct enough as a common util
The current implementation requires the token declaration to be order-dependent, that is, if declared as
```
{
some: '@some2',
some2: '#abc'
}
```
The result will be wrong.
This is not intuitive as a declarative DSL.
Additionally, the code is not necessary to use `Object.prototype` and prefer not to use `startWith` since there is no shim.
A order-independent impl may be like (but **I do not recommend introducing any of these implementation**):
```ts
// But still NO dead loop checker and no array/object deep visit impl yet.
function decodeTokens() {
const stack: string[] = [];
each(tokens.color, (value, key) => {
while (isString(value) && value.indexOf('@') === 0) {
stack.push(key);
key = value.slice(1);
value = tokens.color[key];
}
while (stack.length) {
tokens.color[stack.pop()] = value;
}
});
}
```
### Not necessary as a local impl
It can be simply implemented without "@" grammar:
```
import { extend } from 'zrender/src/core/util';
const tokens = {};
const color = tokens.color = {
theme: [
'#5978e3',
'#b6d634',
'#505372',
'#ff994d',
'#0ca8df',
'#ffd10a',
'#fb628b',
'#785db0',
'#3fbe95'
],
neutral00: '#fff',
neutral05: '#f4f7fd',
neutral10: '#e8ebf0',
neutral15: '#dbdee4',
neutral20: '#cfd2d7',
neutral25: '#c3c5cb',
neutral30: '#b7b9be',
neutral35: '#aaacb2',
neutral40: '#9ea0a5',
neutral45: '#929399',
neutral50: '#86878c',
neutral55: '#797b7f',
neutral60: '#6d6e73',
neutral65: '#616266',
neutral70: '#54555a',
neutral75: '#48494d',
neutral80: '#3c3c41',
neutral85: '#303034',
neutral90: '#232328',
neutral95: '#17171b',
neutral99: '#000',
accent05: '#eef2fc',
accent10: '#dee4f9',
accent15: '#cdd7f7',
accent20: '#bdc9f4',
accent25: '#acbcf1',
accent30: '#9baeee',
accent35: '#8ba1eb',
accent40: '#7a93e9',
accent45: '#6a86e6',
accent50: '#5978e3',
accent55: '#506ccc',
accent60: '#4760b6',
accent65: '#3e549f',
accent70: '#354888',
accent75: '#2d3c72',
accent80: '#24305b',
accent85: '#1b2444',
accent90: '#12182d',
accent95: '#090c17',
transparent: 'rgba(0,0,0,0)',
};
extend(color, {
primary: color.neutral80,
secondary: color.neutral70,
tertiary: color.neutral60,
quaternary: color.neutral50,
disabled: color.neutral20,
border: color.neutral30,
borderTint: color.neutral20,
borderShade: color.neutral40,
background: color.neutral05,
backgroundTint: 'rgba(234,237,245,0.5)',
backgroundTransparent: 'rgba(255,255,255,0)',
backgroundShade: color.neutral10,
shadow: 'rgba(0,0,0,0.2)',
shadowTint: 'rgba(129,130,136,0.2)',
axisLine: color.neutral70,
axisLineTint: color.neutral40,
axisTick: color.neutral70,
axisTickMinor: color.neutral60,
axisLabel: color.neutral70,
axisSplitLine: color.neutral15,
axisMinorSplitLine: color.neutral05,
});
tokens.size = {
xxs: 2,
xs: 5,
s: 10,
m: 15,
l: 20,
xl: 30,
xxl: 40,
xxxl: 50
};
```
I recommend the latter way; do not introduce the complexity.
--
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: commits-unsubscribe@echarts.apache.org
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
Re: [PR] feat: new theme for ECharts 6.0 [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
100pah commented on PR #20865:
URL: https://github.com/apache/echarts/pull/20865#issuecomment-2846630714
@Ovilia
Great design and the very detailed explanation!
And I only list some issues I come up with:
### backward compatibility
A theme of echarts v5 is needed if v6 modify the default settings.
The new design changes numerous default position and size settings.
Users typically use a mix of default settings and custom configurations.
Therefore if upgrading to v6 break the layout and appearance and require significant effort to restore, it may discourage users to upgrade.
### Regarding the colors
I'm just describing my first intuitive feelings - not a professional or serious test, and others may have opposite feelings. Therefore this is just a minor point of reference.
#### The 1st color
I feel the new version is great.
#### The 2nd color
I prefer the v5 slightly, because I feel the new version is a bit too "sharp" or "shiny" (or over saturated?) - especially over large areas. A "shiny" color can feel tiring to look at after a time in front of the screen.
<img width="846" alt="image" src="https://github.com/user-attachments/assets/4b845b05-57e9-4592-8c68-3c66b6dca9e8" />
And a light color make its corresponding label less visible. The label color has the same color if outside, be white color in inside - both of them are less visible:
<img width="958" alt="image" src="https://github.com/user-attachments/assets/6eb15121-fed6-44e9-bd27-626ec1bf8aa4" />
<img width="961" alt="image" src="https://github.com/user-attachments/assets/501b6b9e-5770-42ca-8f31-77d5c1530300" />
#### The 3rd color
The 3rd color has a significant change. I'm worried about the new version is too close to greyscale colors. From my understanding, the greyscale colors are preferable to be used in auxiliary components, such as axis and labels/texts, while chromatic colors are typically used in shapes/glyphs that represents data. All of the other colors seem to conform with that idea, except the new version of the 3rd color. And if it is used over a large area, it gives a feeling that conbinating grayscale and color-scale colors - a bit disharmonious.
#### Regarding the highlight/emphasis color of map
I think this is a significant change from a yellow to grey. I personally strongly prefer the v5 version yellow. That color serves both "hovering-highlight" and "select" - especially in mobile, there is no "hovering" but only "select". If changed to a grey, the selected regions will be less visable. And if only regarding the hovering, the original yellow is noticeable. I think "the yellow is inconsistent with the overall design" is not a concern, since users typically use map individually and do not combinate maps with other types of chart. And actually I can not feel that inconsistency - the original yellow is still harmony.
<img width="885" alt="image" src="https://github.com/user-attachments/assets/a72cc724-6688-41f8-b261-f3d2e8bdba7b" />
--
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: commits-unsubscribe@echarts.apache.org
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