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

Feature/refactor totals #286

Draft
wants to merge 17 commits into
base: develop
Choose a base branch
from
Draft

Feature/refactor totals #286

wants to merge 17 commits into from

Conversation

DarkSide666
Copy link
Member

@DarkSide666 DarkSide666 commented Dec 9, 2017

Total support in Grid is currently quite basic, implementing ability to add up numeric values, define per-column strategies and format the footer.

This PR offers many enhancements:

Ability to add multiple Total lines

Sometimes you would want to define multiple total lines, for instance - SubTotals and Totals. They will be increased simultaneously, but will output in 2 separate lines. They will also have separate strategy.

However SubTotals need ability to be "inserted" multiple times too, which we can implement using hook beforeRow (#277).

$table->addTotals([
   'name'=>'Totals for {$category}',
   'category'=>false,  // will not output and will use colspan
   'amount'=>'sum'
], 'sub');

$last_category = null;
$table->beforeRow(function($row) use ($table) {
    if ($last_category == null) { 
      $last_category = $row['category'];
    } elseif ($last_category != $row['category']) {
      $table->renderTotals('sub', ['category'=>$last_category]);
    }
});

Advanced calculation strategies

Here for the data that contains various dates, we find and display date range in the bottom: "1 Jul - 20 Aug"

$table->addTotals([
  'date'=>[
    'default'=> [null, null],
    'row'=>function($v, $prev) {
        return [min($prev[0], $v), max($prev[1], $v]
    },
    'total'=>function($v) {
        return $v[0]->format('M d').' - '.$v[1]->format('M d');
    },
    'format' => false, // to not use date column formatter
]);

@DarkSide666
Copy link
Member Author

Need to add tests for all different kinds of totals.

@romaninsh
Copy link
Member

wow relally good PR but do I have to write all the docs myself again ? 👍

@DarkSide666
Copy link
Member Author

Tomorrow I will add one more feature there. I have it now (idea) written down on paper but to sleepy to implement it now :)
Also I will write docs with examples tomorrow.

@romaninsh
Copy link
Member

romaninsh commented Dec 12, 2017

something that came to my mind was ability to specify 'name' for the total rows, e.g. addTotals('row2'), then it is safer for you to modify it. Obviously should be optional.

@DarkSide666 DarkSide666 self-assigned this Dec 12, 2017
@DarkSide666
Copy link
Member Author

Also it would be better to move all that totals functionality outside Table in separate class Totals.
That's better because:

  1. make Table class simpler
  2. easier to extend just Totals class and make some changes there
  3. Totals class should be added to Table invisibly for user, so user still will call Table->addTotals(...) and that will do $table->totals_plan[] = new Totals(...);

@romaninsh romaninsh added this to the Someday Maybe milestone Jan 18, 2018
demos/table.php Outdated
return strlen($value) > strlen($total) ? $value : $total;
},
'title'=>function ($total, $model) {
return "Shortes is: ".$total;
Copy link
Member Author

Choose a reason for hiding this comment

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

longest not shortest

docs/table.rst Outdated
$table->addField('no');

$table->addHook('beforeRow', function($table)) {
$table->model['no'] = @++$t->npk;
Copy link
Member Author

Choose a reason for hiding this comment

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

what is $t? Probably $table->nkp, right?

docs/table.rst Outdated
`update` can be string. Supported built-ins are:

- min
- inc
Copy link
Member Author

Choose a reason for hiding this comment

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

inc or incr ?

docs/table.rst Outdated
- avg

You can use a callable which gives you an option to perform update yourself. Also, make note that `inc`
will ONLY increment when value of the column it's associated with is not empty. If you need to get
Copy link
Member Author

Choose a reason for hiding this comment

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

please be more specific what "empty" means here. Is 0 empty? Is empty string empty? Or is it just null. I think that 0 is not empty, but empty string is empty.

docs/table.rst Outdated

`format` uses a template in a format suitable for :php:class:`Template`. Within the template you can
reference other fields too. A benefit for using template is that type formatting is done automatically
for you.

If `format` set to `null` or omitted then default action will be used which is to display totals only
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't understand this sentence.

docs/table.rst Outdated
// $total is previous value
// $value is value of the column
// $model will be loaded to current column
}
Copy link
Member Author

Choose a reason for hiding this comment

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

please add return statement here to make things clearer

src/Table.php Outdated
switch ($f) {
case 'sum':
$this->totals[$key] += $this->model[$key];
// set initial value
$t[$key] = ($t[$key] === null ? 0 : $t[$key]);
Copy link
Member

Choose a reason for hiding this comment

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

looks like you over-engineer here, how about this

$t[$key] = (int)$t[$key] + $this->model[$key];

Copy link
Member

Choose a reason for hiding this comment

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

actually might be tricky because of (int) and (float)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

ok, how about this:

$t[$key] = $t[$key]===null ? 0 : ($t[$key] + $this->model[$key]);

src/Table.php Outdated
// should return new total value (for example, current value + current field value)
// NOTE: Keep in mind, that current total value initially will be null !
if (is_callable($f)) {
$t[$key] = call_user_func_array($f, [$t[$key], $this->model[$key], $key, $this]);
Copy link
Member

Choose a reason for hiding this comment

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

no change needed, but just note on the existence of http://php.net/manual/en/language.oop5.magic.php#language.oop5.magic.invoke

@mvorisek mvorisek marked this pull request as draft April 29, 2020 13:47
@mvorisek mvorisek closed this May 15, 2020
@mvorisek mvorisek reopened this May 15, 2020
@atk4 atk4 deleted a comment from codecov bot Dec 8, 2020
@mvorisek mvorisek removed this from the Someday Maybe milestone Apr 24, 2022
@mvorisek mvorisek force-pushed the feature/refactor-totals branch 2 times, most recently from d43f6e4 to d34d398 Compare April 29, 2023 18:45
@mvorisek
Copy link
Member

mvorisek commented Apr 29, 2023

I have rebased 56da26e 1:1 and simplified the history as much as possible (by squashing CS fixer for example) into d34d398. The diff shows zero changes.

Then I have reverted the changed file paths (files) on the latest develop and rebased the changes again (now with paths matching the current develop): all original changed: c75b7ae...d34d398

@mvorisek mvorisek force-pushed the feature/refactor-totals branch 2 times, most recently from 19f7999 to fe8a82f Compare April 29, 2023 19:29
@mvorisek
Copy link
Member

Hi @DarkSide666, in my post above there is a link to view the real diff - what features/fixes are missing and can you rebase them on the latest develop?

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.

3 participants