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

Add column nesting #98

Open
Kreyu opened this issue Jun 19, 2024 · 3 comments
Open

Add column nesting #98

Kreyu opened this issue Jun 19, 2024 · 3 comments
Assignees
Labels
enhancement Something that would make existing features better

Comments

@Kreyu
Copy link
Owner

Kreyu commented Jun 19, 2024

Currently, the DataTableBuilderInterface::addColumn method currently accepts ColumnBuilderInterface as an argument, but nesting columns is not yet supported.

I'm thinking of some simple, yet friendly API, similar to forms. Let's assume, that we have to render a table like that:

|            |          Quarter           |
|  Employee  | ----------- | ------------ |
|            |      I      |      II      |
| ---------- | ----------- | ------------ |
|  John Doe  |     100     |      50      |
|  Jane Doe  |     200     |      75      |

To achieve such hierarchy, I'm thinking on something like this:

namespace App\DataTable\Type;

use Kreyu\Bundle\DataTableBundle\Column\Type\ColumnType;
use Kreyu\Bundle\DataTableBundle\Column\Type\NumberColumnType;
use Kreyu\Bundle\DataTableBundle\Column\Type\TextColumnType;
use Kreyu\Bundle\DataTableBundle\Type\AbstractDataTableType;
use Kreyu\Bundle\DataTableBundle\DataTableBuilderInterface;

class ProductDataTableType extends AbstractDataTableType
{
    public function buildDataTable(DataTableBuilderInterface $builder, array $options): void
    {
        $builder
            ->addColumn('employee', TextColumnType::class)
            ->addColumn(
                $builder->createColumn('quarter', ColumnType::class)
                    ->addColumn('first', NumberColumnType::class)
                    ->addColumn('second', NumberColumnType::class)
            )
        ;

    }
}

This way, we've created a data table that consists of two columns - employee and quarter, but the latter contains two children columns - respectively first and second.

If you create a view of such data table, you should notice two header rows, with appropriate HTML attributes automatically assigned to the first one:

/** @var \Kreyu\Bundle\DataTableBundle\DataTableView $view */

$view->headerRows[0]['employee']->vars['attr']; // ['rowspan' => 2]
$view->headerRows[0]['quarter']->vars['attr'];  // ['colspan' => 2]

This corresponds to the following HTML structure:

<thead>
    <tr>
        <th rowspan="2">Employee</th>
        <th colspan="2">Quarter</th>
    </tr>
    <tr>
        <th>I</th>
        <th>II</th>
    </tr>
</thead>

This would require following changes:

  1. Adding createColumn() to DataTableBuilderInterface, that returns ColumnBuilderInterface, similar to FormBuilderInterface::create().
  2. Adding column-related methods methods to the ColumnBuilderInterface (addColumn(), removeColumn(), etc.), so API remains consistent. Naming those methods add and remove may seem more intuitive, but I think it would be confusing if used in this case:
$builder
    ->addColumn('employee', TextColumnType::class)
    ->addColumn(
        $builder->createColumn('quarter', ColumnType::class)
            ->add('first', NumberColumnType::class) // "add" what? 
            ->add('second', NumberColumnType::class) // "add" what?
    )
;
  1. Adding a mechanism to automatically apply rowspan and colspan HTML attributes - preferably in the ColumnHeaderView->attr array.
  2. Changing DataTableView to accept array of HeaderRowView, instead of just one.
  3. Some small changes in themes to render multiple headers
  4. Something with personalization - the built-in scripts are using sortablejs, and it already supports nesting! Although, we have to prevent moving nested column outside of their parent.

Some breaking changes, but nothing too extreme. Probably needs more changes, but that's how I see it as today.

@Kreyu Kreyu added the enhancement Something that would make existing features better label Jun 19, 2024
@Kreyu Kreyu self-assigned this Jun 19, 2024
@CMH-Benny
Copy link

One thing to consider on this feature is sorting, In your example you show 2 headers (Employee & Quarter) and Quarter has I & II sub headers, will those be allowed to be sorted as well?

Depending on the data this may become very tricky, same goes for filters and search, so usually it's easier to flatten the table instead.

Also, it might be better to bisect the data per Quarter and show a table per Quarter instead?

Just thinking out loud here, could be a nice addition for some edge cases for sure, but goes more towards a static table, since this kind of nesting can get extremly complex :)

@Kreyu
Copy link
Owner Author

Kreyu commented Jun 28, 2024

One thing to consider on this feature is sorting, In your example you show 2 headers (Employee & Quarter) and Quarter has I & II sub headers, will those be allowed to be sorted as well?

Yeah, I think so. I don't see any risks with that, if you can provide sorting path for such column (e.g. DQL path when working with Doctrine), then there should be no problem with that.

Also, it might be better to bisect the data per Quarter and show a table per Quarter instead?

In some cases, probably. That's just a first example that came to my mind 😄

@Kreyu
Copy link
Owner Author

Kreyu commented Jul 13, 2024

What's currently "blocking" me with this feature, is the column builder's API itself. Similar to many builder classes from Symfony itself, I am naming the last "build" method like a getter. For example, ColumnBuilderInterface currently has a method named getColumn() that creates a ColumnInterface (finishes a building process). This is also the case for DataTableBuilderInterface::getDataTable(), FilterBuilderInterface::getFilter(), and so on, and I think it should stay like that.

This, unfortunately, conflicts with the idea that ColumnBuilderInterface should contain following methods to work on the child columns:

  • getColumn(string $name): ColumnBuilderInterface
  • addColumn(ColumnBuilderInterface|string $column, ?string $type = null, array $options = []): ColumnBuilderInterface
  • and so on...

This was planned to provide a nice looking API for the nested columns:

$builder
    ->addColumn(
        $builder->createColumn('quarter', ColumnType::class) // this returns ColumnBuilderInterface
            ->addColumn('first', NumberColumnType::class) // this addColumn() belongs to ColumnBuilderInterface
            ->addColumn('second', NumberColumnType::class) // this addColumn() belongs to ColumnBuilderInterface
    )
;

We should either use the add, get, remove methods (I really don't like this) similar to FormBuilderInterface:

$builder
    ->addColumn(
        $builder->createColumn('quarter', ColumnType::class)
            ->add('first', NumberColumnType::class)
            ->add('second', NumberColumnType::class) 
    )
;

or name those methods with "nested" or "child" keyword:

$builder
    ->addColumn(
        $builder->createColumn('quarter', ColumnType::class)
            ->addNestedColumn('first', NumberColumnType::class)
            ->addNestedColumn('second', NumberColumnType::class) 
    )
;

Currently, I'm liking the second option more, and it makes this possible:

$builder->getColumn('quarter')->getNestedColumn('first');
$builder->getColumn('quarter')->removeNestedColumn('first');

Unfortunately, more than two-level nesting may look weird when mixing with the DataTableBuilder::createColumn(), as there are no createNestedColumn(), as it would be totally unnecessary:

$builder->addColumn(
    $builder->createColumn('foo')->addNestedColumn(
        $builder->createColumn('bar')->addNestedColumn('baz')
    )
);

// or

$columnFoo = $builder->createColumn('foo');
$columnBar = $builder->createColumn('bar');
$columnBaz = $builder->createColumn('baz');

$columnFoo->addNestedColumn($columnBar);
$columnBar->addNestedColumn($columnBaz);

$builder->addColumn($columnFoo);

But I think that's the compromise we have to make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something that would make existing features better
Projects
None yet
Development

No branches or pull requests

2 participants