Allow Grid::addModalAction() button to be disabled #1868
Conversation
|
Sample usage: |
| @@ -542,9 +542,9 @@ public function addPopup($columnName, $popup = null, $icon = 'caret square down' | |||
| * | |||
| * @return View | |||
| */ | |||
| public function addModalAction($button, $title, \Closure $callback, $args = []) | |||
| public function addModalAction($button, $title, \Closure $callback, $args = [], $isDisabled = false) | |||
There was a problem hiding this comment.
I belive this is not per row, not sure what usecase it should solve.
There was a problem hiding this comment.
Sure, the isDisabled is either boolean or closure, if you pass function($row) { return ($row->get('age') > 12); } you can dynamically set disabled per row as needed. @mvorisek Have many use cases where this is perfect - as it is just extends an already existing feature of addActionButton to addModalAction I'd appreciate if we could merge it
There was a problem hiding this comment.
this need a phpdoc @param above the method like the enabled property phpdoc
https://dev.agiletoolkit.org/demos/tutorial/actions.php?atk_layout_maestro_wizard=3 - UA supports this natively, why do you want to extent this method? Advanced options should not be configureable by a custom parameter. One of my usecases was a different icon for example, should this be addressed by another param? No.
There was a problem hiding this comment.
addAction button has this feature today and all I do is to allow also addModalAction to use this helpful feature. Not everyone is using UA - a lot are using addActionButton etc. to configure Crud which is helpful. I do not see why this is a problem. Or do you want to delete also the isDisabled feature from addActionButton? That would be a pity
There was a problem hiding this comment.
creating UA with a seed should be the desired way to solve *any* related special settings, I am quite against the proposed changes
There was a problem hiding this comment.
Sure we could re-work this to flexibly have multiple advanced parameters - I'd say that would be an idea for future improvements. Alternative could be ActionButtons::onHook(HOOK_GET_HTML_TAGS, function ...) similar to
$table->onHook(Table\Column::HOOK_GET_HTML_TAGS, function (Table $table, Model $row) { ...where we return array of icon, disabled, title
There was a problem hiding this comment.
I do not see why you are against it? There are multiple ways to code, and UA is not always the best way, especially if you want to dynamically adjust Cruds' action buttons in certain views. We should not enforce one single way of coding, but keep room for alternatives, as we all do not have all use cases in mind.
This PR is nothing else than creating consistency between addActionButton and addModalAction with existing features. I am absolutely against removing isDisabled feature unless UA becomes much more easy to code with less lines of code. And even then there might be a lot of reasons why people do not want to use UA but call addActionButton/ addModalAction directly.
There was a problem hiding this comment.
@ibelar Any view on this PR and our pro / con discussion here?
|
rebase in one squashed commit onto develop, narrow \Closure phpdoc types, add Behat test and then let's review this PR |
mvorisek
changed the title
|
@mkrecek234 if you are interested to pursue this PR, let me know, now, I am closing it due inactivity also see #1920 (comment), the "disable action" topic is much more complex and we need to support is uniformly for everything (or nothing) for discussion, there is #1966 issue |
mvorisek
force-pushed
the
feature/action_button_disable
branch
from
a17e5b6 to
1dbe024
Compare
|
I have reworked this PR to be consistent. The idea is good. |
Thank you, @mvorisek |
No description provided.