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/grid crud improvements #156

Merged
merged 5 commits into from
May 12, 2017
Merged

Conversation

romaninsh
Copy link
Member

@romaninsh romaninsh commented May 11, 2017

Grid is now much nicer!!

  • right-aligned
  • buttons easier to click
  • animation if deleting

Known issues:

delete-crud

@romaninsh
Copy link
Member Author

Docs will go through #154

@romaninsh romaninsh mentioned this pull request May 11, 2017
@DarkSide666 DarkSide666 self-requested a review May 12, 2017 05:29
Copy link
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

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

Also not sure about preventDefault. Now it will not be set by default. Is that OK?

src/CRUD.php Outdated
@@ -87,7 +87,7 @@ public function setModel(\atk4\data\Model $m, $defaultFields = null)
$m = parent::setModel($m, $this->fieldsGrid ?: $this->fieldsDefault);

if ($this->can('u')) {
$this->addAction(new Icon('pencil'), new jsModal('Edit', $this->pageEdit, [$this->name=>$this->table->jsRow()->data('id')]));
$this->addAction(['icon'=>'edit'], new jsModal('Edit', $this->pageEdit, [$this->name=>$this->table->jsRow()->data('id')]));
Copy link
Member

Choose a reason for hiding this comment

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

can use new Grid->jsRow() method here

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, correct. i'll update.

@@ -21,7 +27,7 @@ public function addAction($button, $callback)
$this->actions[$name] = $button;
$button->addClass('b_'.$name);
$button->addClass('compact');
$this->table->on('click', '.b_'.$name, $callback);
$this->table->on('click', '.b_'.$name, $callback, [$this->table->jsRow()->data('id'), 'confirm'=>'Are you sure?']);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can use new $grid->jsRow() here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

agree will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm this change no longer there......

Copy link
Member Author

Choose a reason for hiding this comment

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

nevermind.

src/View.php Outdated
@@ -886,7 +909,7 @@ public function on($event, $selector = null, $action = null, $defaults = null)
$this->js(true)->on($event, $action);
}

return $thisAction;
return new jQuery();
Copy link
Member

Choose a reason for hiding this comment

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

really return empty jQuery object?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's what $thisAction was :) not that i think i might have forgotten to add it into $actions[].

if (is_numeric($key)) {
$key = 'c'.$key;
}
$this->args[$key] = $val;
Copy link
Member

Choose a reason for hiding this comment

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

this line should be moved into if statement above

Copy link
Member Author

Choose a reason for hiding this comment

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

no it's designed like this. We keep key as-is if it's symbolic.

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 probably need some tests for that.

Copy link
Member

Choose a reason for hiding this comment

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

yes you are right, sorry. I thought there is foreach($this->args as ...) :)

$response = call_user_func($callback, $chain);

$values = [];
foreach ($this->args as $key=>$value) {
Copy link
Member

Choose a reason for hiding this comment

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

to avoid Avoid unused local variables such as '$value' warning this line should be:
foreach(array_keys($this->args) as $key) {

Copy link
Member Author

Choose a reason for hiding this comment

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

that's more cpu/memory intensive. I prefer to ignore the warning.

Copy link
Member

Choose a reason for hiding this comment

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

ok

if (is_numeric($key)) {
$key = 'c'.$key;
}
$this->args[$key] = $val;
Copy link
Member

Choose a reason for hiding this comment

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

yes you are right, sorry. I thought there is foreach($this->args as ...) :)

@DarkSide666 DarkSide666 merged commit b67ac74 into develop May 12, 2017
@DarkSide666 DarkSide666 deleted the feature/grid-crud-improvements branch May 12, 2017 15:11
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