-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fixes and few new features (insert, delete) #92
base: master
Are you sure you want to change the base?
Conversation
It suppress warning: call_user_func() expects parameter 1 to be a valid callback
Filter buttons 'Filter' and 'Cancel' are renderen in an actions column.
Form can be already customized by renderer. Translator can be already set.
Helps when adding templates on different places. By factory or by descendant __constructor().
😮 wow. thanks, will go through. please ping me often if not :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, though it's getting preeeeetttyyyy complicated. Some doc would be generally needed for the new functionality. (see docs folder in repo)
@@ -91,6 +91,13 @@ | |||
<a href="{link edit! $primary}" class="ajax" data-datagrid-edit>{$control->translate(Edit)}</a> | |||
{/define} | |||
|
|||
{define row-edit-control} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document this.
@@ -20,6 +20,10 @@ | |||
{/if} | |||
{/define} | |||
|
|||
{define row-head-cell} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document this.
@@ -28,6 +28,9 @@ class Column | |||
/** @var Datagrid */ | |||
protected $grid; | |||
|
|||
/** @var array */ | |||
protected $attributes = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea what is the purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example
$grid->addColumn('institute')->setAttribute('help', 'School where you ...');
and then you can render nice help icon in column head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🆗
src/Datagrid.php
Outdated
@@ -487,10 +487,10 @@ public function createComponentForm() | |||
if ($this->filterFormFactory) { | |||
$form['filter'] = call_user_func($this->filterFormFactory); | |||
if (!isset($form['filter']['filter'])) { | |||
$form['filter']->addSubmit('filter', $this->translate('Filter'))->setValidationScope($form['filter']->getControls()); | |||
$form['filter']->addSubmit('filter', 'Filter')->setValidationScope($form['filter']->getControls()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this is probably BC break, should be marked
src/Datagrid.blocks.latte
Outdated
@@ -100,6 +100,31 @@ | |||
<p class="error" n:foreach="$formContainer[$column->name]->getErrors() as $error">{$error}</p> | |||
{/define} | |||
|
|||
{define row-insert-control} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be documented.
@@ -51,6 +52,11 @@ | |||
{ifset #empty-result}{include #empty-result}{/ifset} | |||
{/if} | |||
</tbody> | |||
<tfoot n:ifset="$form[insert]"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it tfoot corrent for inserting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where to render. In header, it can be accidentally interchanged with filter by user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean like last tbody's tr? But probbaly its ok :) /shrug
@@ -540,6 +576,15 @@ public function createComponentForm() | |||
public function processForm(UI\Form $form) | |||
{ | |||
$allowRedirect = true; | |||
if (isset($form['insert']) && $form['insert']['button']->isSubmittedBy()) { | |||
if ($form['insert']['data']->isValid()) { | |||
call_user_func($this->insertFormCallback, $form['insert']['data']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were adding checks elsewhere, but here we are missing, though I'm probably ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check is only on data source callback. Without it, grid cannot be rendered. And when I was beginner with this grid, warning was pretty confusing.
Missing handlers callbacks are IMHO "OK". Warning in production, not exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🆗
src/Datagrid.blocks.latte
Outdated
@@ -111,6 +111,10 @@ | |||
{input button} | |||
{/define} | |||
|
|||
{define row-actions-delete-link} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document.
It's not. INSERT, DELETE and better rendering granularity 😄 |
Thanks for review, but sorry, I have no time to finish it :( |
If you don't mind I will reopen and merge & finish one time :) |
Sure 👍 |
Hi,
would you accept some of proposed features? It is extraction of some which I'm using in local fork.