atk4 / ui

Robust and easy to use PHP Framework for Web Apps
https://atk4-ui.readthedocs.io
MIT License
444 stars 106 forks source link

Column added with Crud::addColumn after Crud::setModel should be added before autocreated column with UAs #1705

Open mkrecek234 opened 2 years ago

mkrecek234 commented 2 years ago

In previous versions, you could easily do for cruds:

$this->addColumn('column1');
$this->addColumn('column1');
parent::setModel($model, []);

This would render then properly having your custom-columns first, and then the Add/Edit user actions next to it. In newest develop branch, this renders an error Different model set if you place setModel after addColum statements.

So question: 1) Change this back again to tolerate this sequence as above? 2) Force that all columns be set with setModel($model, ['column1', 'column2']) and after that customize them one by one using: $this->addDecorator($model->column1, [\Atk4\Ui\Table\Column\Link::class, ['test' => false, 'path' => 'interfaces/page'], ['_id' => 'id']]);

Option 2 is not very elegant for coding though...

mvorisek commented 2 years ago
1. Change this back again to tolerate this sequence as above?

no, Different model set is valid, previously, it was silently ignored

2. Force that all columns be set with setModel($model, ['column1', 'column2']) and after that customize them one by one using:
   `$this->addDecorator($model->column1, [\Atk4\Ui\Table\Column\Link::class, ['test' => false, 'path' => 'interfaces/page'], ['_id' => 'id']]);`

I think we can:

a) modify the Crud::addColumn method so new fields are always move before the autoadded UA column b) provide some method for moving columns, especially the UA column to the end/after some named column.

mkrecek234 commented 2 years ago

@mvorisek An idea: If there are no columns to be rendered since code says parent:setModel($model, []], then Crud also should not add the UserAction buttons column. The buttons column then would need to be added also manually by a command, just like the developer has to addColumns manually. So we just define a function addUserActionColumn() and omit adding it in the setModel in case of empty columns []. Seems very stringent and logic to me. This would even give the developer the freedom to decide if and where to place the UA column easily if he has opted for a manual crud column setup. What do you think?

mvorisek commented 2 years ago

addUserActionColumn seems logical, but it is a BC break and also conditional logic based on [] is very hard for developer to understand/remember.

mkrecek234 commented 2 years ago

Would still see this as the most logical for a case when someone calls setModel with [] columns to customize crud: And code stays compact and clean. Right now you have a lot of coding if you want custom columns as you first pass all columns to setModel, and afterwards you have to decorate/customize each column again.

Your take / community's take. I would vote for it.

mkrecek234 commented 2 years ago

After having worked with it, that means first defining the array of displayed fields (in my case programmatically), then initializing Crud with that field list, and then using addDecorator is perfectly fine, even though seems more lines of code than before. Thus, I close this issue.

mvorisek commented 2 years ago

I faced this issue as well, reopening, maybe we can find some way to fix it.

mvorisek commented 2 years ago

slightly related https://github.com/atk4/ui/issues/1934 (setup methods order should NOT matter)