fix/#746 Duplicate button and icon id

[ibelar]

Fix #746

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (develop@c31574a). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             develop    #747   +/-   ##
=========================================
  Coverage           ?   66.1%           
  Complexity         ?    2226           
=========================================
  Files              ?     118           
  Lines              ?    5243           
  Branches           ?       0           
=========================================
  Hits               ?    3466           
  Misses             ?    1777           
  Partials           ?       0

Impacted Files	Coverage Δ	Complexity Δ
src/TableColumn/Actions.php	92.68% <100%> (ø)	15 <0> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c31574a...eb97119. Read the comment docs.

[pkly]

Will this ensure the element is unique enough? The one genererated in the class attribute is not unique, it repeats every row anyway.

[ibelar]

@pkly - This will ensure that the id attribute for button and icon is set to null and not get duplicate with a generic name like 'atk'. The class attribute name that get duplicate for each button is used as a selector for adding the click handler to the button.

$button->addClass('b_'.$name);
$this->table->on('click', '.b_'.$name, $callback, [$this->table->jsRow()->data('id'), 'confirm' => $confirm]);

This will send the row data-id with each callback when button is click.

Perhaps I do not see the use case to generate a unique id's for each button. Why do you need to set a unique id for each one's in table?

There would be a way to do so but that will require for you to pass your own button object.

For example using your own CRUD class your can override initUpdate and initDelete to set your own button:

$btn = new \atk4\ui\Button(['icon' => 'edit']);
$btn->init();
$btn->id = 'myButton_{$_id}';

$this->addAction($btn, new jsModal('Edit', $this->pageUpdate, [$this->name => $this->jsRow()->data('id'), $this->name.'_sort' => $this->getSortBy()]));

This will generate a template row for the table and the tag {$_id} would be replace with your model id as an example generating something like

<div id="myButton_8" class="ui b_atk_admin_crud_view_table_actions_action_2 compact icon button" style="">
</div>

[pkly]

Perhaps I do not see the use case to generate a unique id's for each button. Why do you need to set a unique id for each one's in table?

Wouldn't that break the actions with on, like

$myButton = $grid->addAction(['icon' => 'whatever']);
$myButton->on('click', function($a) {
   // whatever
});

Don't the js chains rely pretty much only on IDs of elements?
If the js actions can still be attached to this element after this commit I don't really have any issues with it, since IDs of elements are dynamic anyway there's no need for them to exist explicitly I suppose.

[romaninsh]

i think the correct fix would be to make $app->add(['Icon', 'id'=>false]); work correctly. The default value 'atk' should be a property value which can be overridden through a seed.

This annoying bit has been there for some time, but i was afraid to fix due to BC, now is a good time!

[romaninsh]

This can also fix: https://github.com/atk4/ui/blob/develop/src/Form.php#L303

[romaninsh]

implement id=>false properly.

[ibelar]

Perhaps I do not see the use case to generate a unique id's for each button. Why do you need to set a unique id for each one's in table?

Wouldn't that break the actions with on, like

$myButton = $grid->addAction(['icon' => 'whatever']);
$myButton->on('click', function($a) {
   // whatever
});

Don't the js chains rely pretty much only on IDs of elements?
If the js actions can still be attached to this element after this commit I don't really have any issues with it, since IDs of elements are dynamic anyway there's no need for them to exist explicitly I suppose.

I see, although $grid->addAction($name, $function) return a button and that you can still add a js_action to it. However, js_action added to it will not be render, probably because Table is in fact a Lister and the button is not an element of the table directly. I guess the only reason why a button is returned via Grid::addAction() method is to be able to modify button look.
Hope this help clarified. But you where still right in first place about duplicate id that should not get generated.

[romaninsh]

We can use custom selector on jsAction. Something similar to Field->jsInput()

[ibelar]

i think the correct fix would be to make $app->add(['Icon', 'id'=>false]); work correctly. The default value 'atk' should be a property value which can be overridden through a seed.

This annoying bit has been there for some time, but i was afraid to fix due to BC, now is a good time!

@romaninsh
Fix:

• Can now use $app->add(['Icon', 'id'=>false]);
• View now has defaultName property set to 'atk' that can be override.

[romaninsh]

This is perfect, thanks!