Conversation
lippserd
left a comment
There was a problem hiding this comment.
- Please always use single quotes for strings that don't require interpolation. This helps to distinguish strings that actually require it.
- Use heredoc/nowdoc for expected HTML.
- Always add a newline at the end of files. PHPStorm has a setting for this. Also, PHPStorm can automatically remove spaces at the end of lines.
- I would go with the following API:
setTriggerElement()instead ofsetAddTrigger().setRemoveElement()instead ofsetRemoveTrigger().onTrigger()instead ofon(Collection::ON_LOAD, ...).
- Remove events as they become obsolete with the above changes.
2c3011d to
1c6d79c
Compare
| ); | ||
| } | ||
|
|
||
| $this |
There was a problem hiding this comment.
Please use the following:
$this
->assembleGroup($group, $addElement ?? null, $removeElement ?? null)
->addElement($group);
Else the decorate() call is missing.
There was a problem hiding this comment.
With addElememt() the items added from assembleGroup are no longer added. I tried adding ->decorate($group) before the addHtml call, and the output was the same in my test cases, also in the Unit Tests.
|
|
||
| if ($valid) { | ||
| $lastKey = $values ? key(array_slice($values, -1, 1, true)) + 1 : 0; | ||
| $this->addGroup($lastKey); |
There was a problem hiding this comment.
Could this be just $this->addGroup($key ? ($key + 1): 0);?
There was a problem hiding this comment.
The problem is getting the last key in the first place. Since this is not the foreach anymore. Functionality wise this should act like array_key_last, which is only available in 7.3 and up.
|
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @lippserd, @TAINCER Details
|
|
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @lippserd, @TAINCER Details
|
|
@cla-bot check |
d7c0d60 to
599c4f6
Compare
fbac7d9 to
b54c353
Compare
lippserd
left a comment
There was a problem hiding this comment.
Please rebase with master and add proper class and method PHPDoc. Also example usage for the element would be nice.
…ndCallbacks()` to `Attributes`
This aims to add support for dynamic Form Fields. The API is still WIP, up to date examples can be found in
CollectionTest.php.Simple Example:
depends on