LaravelDaily / laravel-charts

Package to draw charts in Laravel with Chart.js
MIT License
522 stars 116 forks source link

Support nested properties #88

Closed NathanaelGT closed 1 year ago

NathanaelGT commented 2 years ago

This PR adds support for nested properties so the developers can show nested properties using dot notation

use case

$options = [
    'chart_title' => 'User Roles',
    'report_type' => 'group_by_string',
    'model' => User::class,
    'group_by_field' => 'role.value', // enum
    'chart_type' => 'pie',
];
PovilasKorop commented 2 years ago

@NathanaelGT thanks, currently very limited time, but will somehow find the time to test and merge, in upcoming week or so.

krekas commented 2 years ago

@NathanaelGT if I understand correctly lets say I have 20 users with roles admin and user and group_by_field is role.name pie chart should have two slices right? One with count how many admins and other for users. If so for me doesn't work.

NathanaelGT commented 2 years ago

@krekas I have tested it locally and it works, may I have a look at your code?

Here is my demo project: url

krekas commented 2 years ago

@NathanaelGT it's default project from quickadminpanel and roles relation is

public function roles()
{
    return $this->belongsToMany(Role::class);
}

so nested works with One To Many?

NathanaelGT commented 2 years ago

It works with One To Many (Inverse)

NathanaelGT commented 2 years ago

It should work with Many To Many and One To Many now @krekas

NathanaelGT commented 2 years ago

Any news about this PR?

PovilasKorop commented 2 years ago

@NathanaelGT sorry for the delay, I'm on vacation and colleague @krekas is busy on other tasks.

The problem with this PR that it changes the fundamental logic of the data for ALL cases - different chart types, different relationships, etc. So it's time-consuming just to TEST all possible cases if it doesn't break any of them. That's why we're "procrastinating" on it.

In your demo project, I see just one simple case tested: https://github.com/NathanaelGT/laravel-charts_demo-88/blob/main/app/Http/Controllers/ChartController.php

So one of us would need to work to test other possible scenarios. Choose from these options:

  1. You expand your demo project to other charts, covering belongsTo, belongsToMany, and different models, and a few types of charts - pie, line, bar. And you write automated tests for them that the data is returned successfully and doesn't break
  2. Or, you wait for me to find the time to do the same, realistically at the end of August, only. Unfortunately, this package is not a priority for us at all, a huge pile of other more priority work waiting.
NathanaelGT commented 2 years ago

I will take option 1, maybe I will have some time around the beginning of August

PovilasKorop commented 1 year ago

@NathanaelGT sorry closing this as inactive. If you want, in the future please repeat the PR with tests, so it would be quicker for us to review.