Upgrade chart.js to the latest version #22
Conversation
Co-authored-by: Michael Voříšek <[email protected]>
Co-authored-by: Michael Voříšek <[email protected]>
| 'tooltip' => [ | ||
| 'enabled' => true, | ||
| 'mode' => 'point', | ||
| 'callbacks' => [ |
There was a problem hiding this comment.
in atk4/* we prefer as much as possible things to be done in php, here format the values using atk4/ui UI persistence - can we do that?
There was a problem hiding this comment.
Maybe we can with some workaround (like first storing all labels as JS array filled from PHP side) and then in JS callback lookup that value, but I don't think it's worth it. This way it's easy and straightforward at least for now.
There definitely is space for improvement - maybe someone who use charts in projects will contribute something nicer with a time, but for now ... i guess it's fine like this.
| */ | ||
| protected function prepareDatasets(): void | ||
| { | ||
| if ($this->model === null || $this->columns === null) { |
There was a problem hiding this comment.
Is model required to be set or not?
There was a problem hiding this comment.
Yes model and columns are required. Do you think exception here would be better?
| public function setColumnOptions(array $options) | ||
| { | ||
| // IMPORTANT: use replace not merge here to preserve numeric keys !!! | ||
| $this->columnOptions = array_replace_recursive($this->columnOptions, $options); |
There was a problem hiding this comment.
array_replace_recursive function is highly problematic, see atk4/ui#1811 and related data bug
why we need it and do you understand all the side effects? I prefer to never use this function across all atk4 repos
There was a problem hiding this comment.
Not sure about all side effects, but replace was needed here to be able to merge array elements with numeric keys. array_merge_recursive appends (not merge/replace) elements with numeric keys.
|
Thank you @DarkSide666! |
NOTE: There are BC incompatible changes !