Skip to content

Conversation

@kurkle
Copy link
Member

@kurkle kurkle commented Feb 27, 2021

Combine controllers[type].datasets into datasets[type], for simpler options lookup and consistent defaults/options.

@kurkle
Copy link
Member Author

kurkle commented Feb 27, 2021

@stockiNail I'd appreciate any comments on this one, I'm not so sure if it should actually be the other way around, or should it not be changed at all.

@stockiNail
Copy link
Contributor

@stockiNail I'd appreciate any comments on this one, I'm not so sure if it should actually be the other way around, or should it not be changed at all.

@kurkle I had a look and this sounds to me more consistent comparing with previous implementation.
Of course someone could complain that the datasets are not part of the controller but from user perspective it could be more clear (at least to me).

@stockiNail
Copy link
Contributor

stockiNail commented Feb 28, 2021

@kurkle I thought a bit more about the topic.
Well, my perspective is that datasets namespace is part of the controller therefore I'd expect to have the following structure/fallback:

dataset // dataset object
!
+--> options.datasets // datasets of the controller of the instantiated chart
     !
     +--> controllers[type].datasets // datasess default for the controller
           !
           +--> defaults.datasets // defaults for datasets of all controller type (maybe useless)

This chain should be also "merged" (or used by proxy) quite easily I guess.
But there is a use case (and I think only one) where it doesn't work as is, the combined chart, where you have datasets related to different controllers.

Probably to maintain the datasets logic as is, you should use the implementation of this PR where the datasets namespace is containing all controller types and their datasets options.

Going to this PR, I have only a doubt about custom controllers which are extending existing ones if the defaults.datasets[newController] must be set by the user/implementation before the controller registration.

@kurkle
Copy link
Member Author

kurkle commented Mar 2, 2021

@kurkle I thought a bit more about the topic.
Well, my perspective is that datasets namespace is part of the controller therefore I'd expect to have the following structure/fallback:

dataset // dataset object
!
+--> options.datasets // datasets of the controller of the instantiated chart
     !
     +--> controllers[type].datasets // datasess default for the controller
           !
           +--> defaults.datasets // defaults for datasets of all controller type (maybe useless)

I'm not sure what benefit would options.datasets bring compared to options?
options.datasets.line I understand on a mixed chart, but the plain datasets is just one more level to define something that affects the whole chart.

This chain should be also "merged" (or used by proxy) quite easily I guess.
But there is a use case (and I think only one) where it doesn't work as is, the combined chart, where you have datasets related to different controllers.

As you know, the current fallback is

  • dataset
  • options.datasets[type]
  • options.controllers[type].datasets
  • options
  • defaults.datasets[type}
  • defaults.controllers[type].datasets
  • defaults

The two, options.datasets[type] and options.controllers[type].datasets are equal in terms of where the options end up in.

Probably to maintain the datasets logic as is, you should use the implementation of this PR where the datasets namespace is containing all controller types and their datasets options.

Thats the idea in this PR.

Going to this PR, I have only a doubt about custom controllers which are extending existing ones if the defaults.datasets[newController] must be set by the user/implementation before the controller registration.

The customController.defaults.datasets is automatically merged into defaults.datasets[type], in this PR, so should not be a problem.

@stockiNail
Copy link
Contributor

I'm not sure what benefit would options.datasets bring compared to options?
options.datasets.line I understand on a mixed chart, but the plain datasets is just one more level to define something that affects the whole chart.

I fully agree with you that sounds useless. As you highlighted, the only exceptions is mixed chart.

The two, options.datasets[type] and options.controllers[type].datasets are equal in terms of where the options end up in.

Honestly I wasn't aware about options.controllers[type] but I think options.datasets[type] and options.controllers[type].datasets should be the same. But I think that also defaults.datasets[type} and defaults.controllers[type].datasets should be the same.

Maybe I don't have the clear picture but the above objects could be "different" only if *.controllers[type].dataset is a read only object used only to maintain the original defaults of the controller. But maybe I'm wrong.

defaults.datasets[type}.stock = 1
defaults.controllers[type].datasets.stock = 1;
// the above statements should apply the same behavior to the chart
// and to change the first or the second statement is the same

What is not clear to me, why should a user change defaults.datasets[type} instead of defaults.controllers[type].datasets ?

@stockiNail
Copy link
Contributor

@kurkle additional thought:

  • dataset <- to configure a single datasets of the chart
  • options.datasets[type] <- to configure all datasets of the same controller type of the chart
  • options.controllers[type].datasets <- to configure all dataset of the same controller type of the chart (the same of above)
  • options <- to configure everything of the chart
  • defaults.datasets[type} <- to configure all datasets of the same controller type for all charts
  • defaults.controllers[type].datasets <- to configure all datasets of the same controller type for all charts (the same of above)
  • defaults <- to configure everything for all charts

If I have understood well the PR, this is new fallback:

  • dataset <- to configure a single datasets of the chart
  • options.datasets[type] <- to configure all datasets of the same controller type of the chart
  • options <- to configure everything of the chart
  • defaults.datasets[type} <- to configure all datasets of the same controller type for all charts
  • defaults <- to configure everything for all charts

New PR sounds to me more consistent.
In this way you should address the mixed chart and also the redundancy of fallback on the objects.

Does it mean that *.controllers[type].datasets will be removed or ignored having the default controllers config in the defaults.datasets[type] instead of defaults.controllers[type].datasets?

@kurkle
Copy link
Member Author

kurkle commented Mar 2, 2021

Does it mean that *.controllers[type].datasets will be removed or ignored having the default controllers config in the defaults.datasets[type] instead of defaults.controllers[type].datasets?

Yes, it would mean that.

@kurkle
Copy link
Member Author

kurkle commented Mar 2, 2021

I still have some doubts. While datasets[type] is shorter and somewhat more obvious, the element defaults / options per controller would are still in controllers[type].elements[el]. And controller level plugin defaults similar in controller[type].plugins[id]. So it is actually less consistent. The other option would be to remove the datasets[type] and only read the controllers[type].datasets.

@stockiNail
Copy link
Contributor

I still have some doubts. While datasets[type] is shorter and somewhat more obvious, the element defaults / options per controller would are still in controllers[type].elements[el]. And controller level plugin defaults similar in controller[type].plugins[id]. So it is actually less consistent. The other option would be to remove the datasets[type] and only read the controllers[type].datasets.

In my opinion, elements and plugins are not strictly related to the controller type. Or better element is related to element type (and a controller could use more than 1 element, like line controller) therefore it could make more sense.

I'm thinking loud, removing datasets[type], the fallback could be:

  • dataset <- to configure a single datasets of the chart
  • options.controllers[type].datasets <- to configure all dataset of the same controller type of the chart (the same of above)
  • options <- to configure everything of the chart
  • defaults.controllers[type].datasets <- to configure all datasets of the same controller type for all charts (the same of above)
  • defaults <- to configure everything for all charts

In this case we are not consistent as well (my opinion), as you mentioned about other namespace, like elements which are defined in options and defaults as well.

Probably the best is the current one, maybe a little bit redundant. ;)

@etimberg
Copy link
Member

etimberg commented Mar 2, 2021

I think after defaults.controllers[type].datasets we still need something for type specific chart level options because not all options are configurable per dataset.

@kurkle
Copy link
Member Author

kurkle commented Mar 2, 2021

@etimberg I think those are (mostle at least) at controllers[type]

@etimberg
Copy link
Member

etimberg commented Mar 2, 2021

Yup, that's where they are now. I just want to make sure any proposal keeps the fallback there

@kurkle kurkle requested review from benmccann and simonbrunel March 3, 2021 13:56
@benmccann
Copy link
Contributor

I think the original reason for putting these options in controllers[type].datasets was that we wanted to have all options for a chart type grouped together. E.g. anything affecting a bar chart should be able to be found under controllers.bar. Though it looks like now maybe we also have datasets[type] and it does seem like we shouldn't need both. What's currently set via datasets[type]?

@kurkle
Copy link
Member Author

kurkle commented Mar 4, 2021

Closing in favor of #8563

@kurkle kurkle closed this Mar 4, 2021
@kurkle kurkle deleted the ds-opts branch March 9, 2021 17:36
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.

4 participants