elastic / elastic-charts

https://elastic.github.io/elastic-charts/storybook
Other
370 stars 120 forks source link

Sorting legend does not work correctly in some cases #1685

Closed qn895 closed 2 years ago

qn895 commented 2 years ago

Describe the issue

Sorting legend does not work correctly in some cases when there are Area series preceding Line series (or possibly when there are simply multiple series?)

To Reproduce

Case 1:

  1. Go to https://codesandbox.io/s/xenodochial-ptolemy-w5ftu4?file=/src/App.tsx
  2. Toggle between "Enable legendSort" & "Disable legendSort" button
  3. Note that the legend is being sorted correctly/as expected. When sortLegendEnabled === true, Expected bounds should be after Throughput.

Case 2:

  1. Go to https://codesandbox.io/s/recursing-euclid-rjr3x0?file=/src/App.tsx
  2. Note that the legend is NOT being sorted. Even though it uses the same legendSort function as Case 1, Expected bounds still show up before Throughput.

Checklist

markov00 commented 2 years ago

Hi @qn895 sorry for reaching out on this so late. I don't see a problem in our code actually, the problem is in the way you are defining the sorting function. Our sort function works exactly the same as the JS Array sort function

In your examples you are returning -1 independently for the variable a or the the variable b:

if ((a as XYChartSeriesIdentifier)?.specId === "Expected bounds") return -1;
if ((b as XYChartSeriesIdentifier)?.specId === "Expected bounds") return -1;
return 1;

The sort function dones't work well this way, because if you want to sort a before b you must put a negative number, but if you want to sort b before a you must send a positive value. (see the table here)

So in your case, the best way to fix that is to return the following:

if ((a as XYChartSeriesIdentifier)?.specId === "Expected bounds") return -1; // sort a before b where a is Expected bounds
if ((b as XYChartSeriesIdentifier)?.specId === "Expected bounds") return 1; // sort b before a where b is Expected bounds
return 0; // keep the original order for everything else

Please reopen this issue if you notice something else