Skip to content
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

Implement dataset.order #6268

Merged
merged 6 commits into from Oct 22, 2019
Merged

Implement dataset.order #6268

merged 6 commits into from Oct 22, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented May 12, 2019

Resolves: #6050
Resolves: #5171

Pen (updated!)

docs/charts/mixed.md Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor

My personal opinion is that it would be clearer and more flexible to use z-index here and mix the datasets with the other layers. I understand there's some concern that we fire beforeDatasetsUpdate render all the datasets and then fire afterDatasetsUpdate with nothing else happening in between. However, I'm not sure why we need to enforce that vs documenting the effect of the z-layer option and making the user aware that if you have z-indexes for other layers that are between dataset z-indexes those renderings will happen in between beforeDatasetsUpdate and afterDatasetsUpdate. To me, it's confusing, unnecessary, and restrictive to have multiple ordering concepts. Ultimately it should be under the user's control. If someone wants to draw one dataset, then the gridlines, then another dataset and they aren't using any plugins which that would cause problems for, then I don't see why we should stop them

etimberg
etimberg previously approved these changes May 13, 2019
@kurkle
Copy link
Member Author

kurkle commented May 18, 2019

@benmccann after 74ff647 this order lets you change the stacking order of datasets too. z-index doesn't help there. There is no reason why z-index could not be added on top of this.

@benmccann
Copy link
Contributor

Maybe I don't understand what stacking order of datasets is. I thought it was the order in which datasets were drawn similar to z-index. I don't think you could have both stacking order and z-index because they're essentially the same thing and what if you specified conflicting orders?

@simonbrunel
Copy link
Member

order and z are different features: order determines the sorting of the dataset array, which impacts the drawing order but also the stacking (scale.stacked: true), tooltip and legend, while z only impacts the drawing order. Note that order doesn't change the z index of the dataset and is actually compatible with it (i.e. datasets at the same z would be sorted using order).

z is breaking while order is not and has been requested many times, long time ago. I think z for datasets should be delayed to v3 and we should get order in v2 to already unlock many use cases.

Basically, order is similar to:

new Chart(ctx, {
  data: {
    datasets: [
      { ... },
      { ... },
      //...
    }].sort(dataset => dataset.order)
})

... which I think should be the responsibility of the user to sort the array before creating/updating the chart but too many people asked for something built-in. Though, in v3, we should convert the dataset array to an object keyed by the dataset id, in which case, order would make more sense:

// V3 datasets
new Chart(ctx, {
  datasets: {
    line0: { order: 1 },
    line1: { order: 0 },
    // ...
  }
})

@benmccann it's confusing, unnecessary, and restrictive to have multiple ordering concepts

I tend to think that it's less confusing. Most use cases have all datasets before, after or between scale components (but not mixed). In this case, order makes easier to sort datasets instead to have to worry about other layers (some could come from plugins).

@benmccann I don't see why we should stop them

I agree but to me it's a v3 feature (which hasn't been requested yet)

@benmccann
Copy link
Contributor

Ok. Makes sense to me. Thanks for the additional explanation. This PR looks pretty good to me

@kurkle
Copy link
Member Author

kurkle commented May 21, 2019

Rebased and addressed review comments

@Aka4Code
Copy link

When do you plan to release this feature?
Do you think soon there will be 2.8.1 version with this?

With version 2 there will be only with z feature?
There is no chance to release both order and z?
We need to wait for version 3 for z ?

@benmccann
Copy link
Contributor

@kurkle it looks like this PR will need to be rebased

@Aka4Code the next release will probably be 2.9.0. I don't have an ETA for you

@kurkle
Copy link
Member Author

kurkle commented Jun 25, 2019

Rebased

Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

Looks good except for a few minor things.

docs/charts/mixed.md Outdated Show resolved Hide resolved
docs/charts/bar.md Outdated Show resolved Hide resolved
@kurkle
Copy link
Member Author

kurkle commented Jul 1, 2019

Updated based on comments

docs/charts/bar.md Outdated Show resolved Hide resolved
src/scales/scale.linear.js Outdated Show resolved Hide resolved
src/controllers/controller.bar.js Outdated Show resolved Hide resolved
src/scales/scale.linear.js Outdated Show resolved Hide resolved
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Allow sorting datasets based on the `order` property
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to reverse render order of datasets Bar data overflow Line data
8 participants