Skip to content

fix(dataZoom): data shape distribution for time axis #16978

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 17, 2025

Conversation

andrearoota
Copy link
Contributor

@andrearoota andrearoota commented May 4, 2022

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Fix render data shadow line with data not equally distributed in time axis

Fixed issues

#16924
#15921
#16980

Details

Before: What was the problem?

Data shadow line in bottom dataZoom does not match with serie line on graph

image

After: How is it fixed in this PR?

If type of axis is set to "time", scale the original axis data range to dataZoom axis range
I used this mathematical formula

image

Misc

  • The API has been changed (apache/echarts-doc#xxx).
  • 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

@echarts-bot
Copy link

echarts-bot bot commented May 4, 2022

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

@andrearoota andrearoota changed the title fix(dataZoom): render data not equally distributed. close apache#16924 fix(dataZoom): render data not equally distributed. close #16924 #15921 May 4, 2022
@Ovilia
Copy link
Contributor

Ovilia commented Sep 2, 2022

I'm also fixing related problems at #17311 . I will check the logic more carefully later. Thanks for your contribution!

@Ovilia Ovilia removed this from the 5.4.1 milestone Sep 23, 2022
@piotrbubelE
Copy link

Hello! Is there a plan to merge this PR anytime soon?
It also enables workaround for bug with axis.min and axis.max on dataShadow

@pchrzaszczewski
Copy link

I am also waiting for this fix. What are your plans?

@kamilapolewczykatos
Copy link

Merge would be great. I need this fix for my app.

@zsamotsil
Copy link

zsamotsil commented May 20, 2024

It would be great to ship it. Need also this fix in my application.

@piotrbubelE
Copy link

Here is issue that can use workaround from this PR:
#19957

@grzesikj
Copy link

When will this be available for use? Without this I cannot use data zoom slider in my project.

@Bernard-code
Copy link

I really could use this fix. When will this be available to use?

@grzesikj
Copy link

grzesikj commented Aug 9, 2024

Add it, please.

@grzesikj
Copy link

When can you add it?

@noahhai
Copy link

noahhai commented Sep 25, 2024

Fix PR pending for two years 😭

@pchrzaszczewski
Copy link

Should we spam somehow this topic in anticipation of the answer?
Echarts team give us some answer.

I give you some possible options. I hope it will help you to respond to the community:

  • "we are already working on it, fix will be available in... insert date here",
  • "it was solved differently, here you have the solution...",
  • "we don't care",
  • "we don't give a shit",
  • "go to hell"

Choose wisely. We will know what to do next... 😞

@noahhai
Copy link

noahhai commented Sep 26, 2024

@pchrzaszczewski Woah easy there. I get the frustration though.

I am going to create a pnpm patch based on the fixes in the PR and just pin the echarts version in my dependencies. Not great, but I assume at some point it will get fixed.

@andrearoota
Copy link
Contributor Author

@pchrzaszczewski Woah easy there. I get the frustration though.

I am going to create a pnpm patch based on the fixes in the PR and just pin the echarts version in my dependencies. Not great, but I assume at some point it will get fixed.

Does my code still work in the current version? I am willing to improve it and update it if necessary

@pchrzaszczewski
Copy link

@pchrzaszczewski Woah easy there. I get the frustration though.

I am going to create a pnpm patch based on the fixes in the PR and just pin the echarts version in my dependencies. Not great, but I assume at some point it will get fixed.

@noahhai I'm grateful that you responded. We can see that something is happening ;)

I've been patient for 2 years 💪 It is always better to at least know if there is a plan or not.
I'm keeping my fingers crossed and cheering that the day will finally come... 🤞

@grzesikj
Copy link

grzesikj commented Nov 7, 2024

I'm still waiting

@alinscodes
Copy link

Can we just deploy this pleasE?

Copy link

@alinscodes alinscodes left a comment

Choose a reason for hiding this comment

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

This is good.

@jteez
Copy link

jteez commented Dec 11, 2024

Still hoping for this fix too!

@ElicaInc
Copy link

ElicaInc commented Apr 14, 2025

@andrearoota fixed the mismatch between the chart and dataZoom. It works well in my setup—thank you, @andrearoota!

@alinscodes
Copy link

Lets get this in please

@ElicaInc
Copy link

This patch, made possible thanks to @andrearoota, worked perfectly in my setup while waiting for the official merge.

diff --git a/node_modules/echarts/lib/component/dataZoom/SliderZoomView.js b/node_modules/echarts/lib/component/dataZoom/SliderZoomView.js
index be21afa..25e561e 100644
--- a/node_modules/echarts/lib/component/dataZoom/SliderZoomView.js
+++ b/node_modules/echarts/lib/component/dataZoom/SliderZoomView.js
@@ -281,22 +281,35 @@ var SliderZoomView = /** @class */function (_super) {
       var areaPoints_1 = [[size[0], 0], [0, 0]];
       var linePoints_1 = [];
       var step_1 = thisShadowExtent[1] / (data.count() - 1);
-      var thisCoord_1 = 0;
+        // Added: Variables to support time axis
+      var thisDataExtent = data.getDataExtent(info.thisDim);
+      var normalizationConstant = size[0] / (thisDataExtent[1] - thisDataExtent[0]);
+      var isTimeAxis = info.thisAxis.type === 'time';
+      var thisCoord_1 = -step_1;
       // Optimize for large data shadow
       var stride_1 = Math.round(data.count() / size[0]);
       var lastIsEmpty_1;
-      data.each([otherDim], function (value, index) {
-        if (stride_1 > 0 && index % stride_1) {
-          thisCoord_1 += step_1;
+      data.each([info.thisDim, otherDim], function (thisValue, otherValue, index) {
+        if (stride_1 > 0 && (index % stride_1)) {
+          // Added: Only increment step when not using a time axis
+          if (!isTimeAxis) {
+            thisCoord_1 += step_1;
+          }
           return;
         }
+        
+        // Fix: Coordinate calculation split between time axis and non-time axis
+        thisCoord_1 = isTimeAxis
+          ? (+thisValue - thisDataExtent[0]) * normalizationConstant
+          : thisCoord_1 + step_1;
+    
         // FIXME
         // Should consider axis.min/axis.max when drawing dataShadow.
         // FIXME
         // 应该使用统一的空判断?还是在list里进行空判断?
-        var isEmpty = value == null || isNaN(value) || value === '';
+        var isEmpty = otherValue == null || isNaN(otherValue) || otherValue === '';
         // See #4235.
-        var otherCoord = isEmpty ? 0 : linearMap(value, otherDataExtent_1, otherShadowExtent_1, true);
+        var otherCoord = isEmpty ? 0 : linearMap(otherValue, otherDataExtent_1, otherShadowExtent_1, true);
         // Attempt to draw data shadow precisely when there are empty value.
         if (isEmpty && !lastIsEmpty_1 && index) {
           areaPoints_1.push([areaPoints_1[areaPoints_1.length - 1][0], 0]);
@@ -307,7 +320,7 @@ var SliderZoomView = /** @class */function (_super) {
         }
         areaPoints_1.push([thisCoord_1, otherCoord]);
         linePoints_1.push([thisCoord_1, otherCoord]);
-        thisCoord_1 += step_1;
+        
         lastIsEmpty_1 = isEmpty;
       });
       polygonPts = this._shadowPolygonPts = areaPoints_1;

@carlos-manuel-soares-alb

Any news for this PR to be merged?

Copy link

echarts-bot bot commented May 17, 2025

To reviewers: If this PR is going to be described in the changelog in the future release, please make sure this PR has one of the following labels: PR: doc ready, PR: awaiting doc, PR: doc unchanged

This message is shown because the PR description doesn't contain the document related template.

@100pah 100pah merged commit fc6656f into apache:master May 17, 2025
Copy link

echarts-bot bot commented May 17, 2025

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

Ovilia added a commit that referenced this pull request Jun 18, 2025
fix(dataZoom): fix cases with dataset, relates to #16978
Ovilia added a commit that referenced this pull request Jun 19, 2025
@Ovilia Ovilia added this to the 6.0.0 milestone Jun 19, 2025
@Ovilia Ovilia changed the title fix(dataZoom): render data not equally distributed. close #16924 #15921 fix(dataZoom): data shape distribution for time axis Jun 21, 2025
Ovilia added a commit that referenced this pull request Jun 21, 2025
fix(dataZoom): fix the case when first value is NaN, relates to #16978
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet