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

Fixes and few new features (insert, delete) #92

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

milo
Copy link
Contributor

@milo milo commented May 25, 2018

Hi,
would you accept some of proposed features? It is extraction of some which I'm using in local fork.

@milo milo changed the title Fixes and few new features (insert, create) Fixes and few new features (insert, delete) May 25, 2018
@hrach
Copy link
Member

hrach commented May 25, 2018

😮 wow. thanks, will go through. please ping me often if not :D

Copy link
Member

@hrach hrach left a 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}
Copy link
Member

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}
Copy link
Member

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 = [];
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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());
Copy link
Member

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

@@ -100,6 +100,31 @@
<p class="error" n:foreach="$formContainer[$column->name]->getErrors() as $error">{$error}</p>
{/define}

{define row-insert-control}
Copy link
Member

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]">
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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']);
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

🆗

@@ -111,6 +111,10 @@
{input button}
{/define}

{define row-actions-delete-link}
Copy link
Member

Choose a reason for hiding this comment

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

Document.

@milo
Copy link
Contributor Author

milo commented May 25, 2018

preeeeetttyyyy complicated

It's not. INSERT, DELETE and better rendering granularity 😄

@milo
Copy link
Contributor Author

milo commented Jul 11, 2018

Thanks for review, but sorry, I have no time to finish it :(

@milo milo closed this Jul 11, 2018
@hrach
Copy link
Member

hrach commented Jul 11, 2018

If you don't mind I will reopen and merge & finish one time :)

@hrach hrach reopened this Jul 11, 2018
@milo
Copy link
Contributor Author

milo commented Jul 12, 2018

Sure 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants