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

Add Collection for dynamic Forms #71

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Add Collection for dynamic Forms #71

wants to merge 20 commits into from

Conversation

TAINCER
Copy link
Contributor

@TAINCER TAINCER commented Aug 25, 2022

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:

        $collection = new Collection('testCollection');

        $collection->setAddElement('add_element', [
            'required' => false,
            'label'    => 'Add Trigger',
            'options'  => [null => 'Please choose', 'first' => 'First Option'],
            'class'    => 'autosubmit'
        ]);

        $collection->onAssembleGroup(function ($group, $addElement, $removeElement) {
            $group->addElement($addElement);

            $group->addElement('input', 'test_input');
        });

        $form
            ->registerElement($collection)
            ->addHtml($collection)

depends on

@TAINCER TAINCER self-assigned this Aug 25, 2022
@cla-bot cla-bot bot added the cla/signed label Aug 25, 2022
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

  • 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 of setAddTrigger().
    • setRemoveElement() instead of setRemoveTrigger().
    • onTrigger() instead of on(Collection::ON_LOAD, ...).
  • Remove events as they become obsolete with the above changes.

src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
@TAINCER TAINCER requested a review from lippserd October 6, 2022 07:50
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Show resolved Hide resolved
src/FormElement/Collection.php Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
src/FormElement/Collection.php Outdated Show resolved Hide resolved
);
}

$this
Copy link
Member

Choose a reason for hiding this comment

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

Please use the following:

        $this
            ->assembleGroup($group, $addElement ?? null, $removeElement ?? null)
            ->addElement($group);

Else the decorate() call is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

src/FormElement/Collection.php Outdated Show resolved Hide resolved

if ($valid) {
$lastKey = $values ? key(array_slice($values, -1, 1, true)) + 1 : 0;
$this->addGroup($lastKey);
Copy link
Member

Choose a reason for hiding this comment

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

Could this be just $this->addGroup($key ? ($key + 1): 0);?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@nilmerg nilmerg added this to the v0.7.0 milestone Oct 18, 2022
@nilmerg nilmerg added the enhancement New feature or request label Nov 8, 2022
@cla-bot
Copy link

cla-bot bot commented Nov 24, 2022

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

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@cla-bot cla-bot bot removed the cla/signed label Nov 24, 2022
@cla-bot
Copy link

cla-bot bot commented Nov 24, 2022

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

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@bobapple
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Nov 24, 2022
@lippserd lippserd marked this pull request as ready for review January 16, 2023 10:11
@nilmerg nilmerg changed the base branch from master to cloning February 6, 2023 09:56
@TAINCER TAINCER changed the base branch from cloning to fix-cloning February 6, 2023 10:12
@TAINCER TAINCER force-pushed the Collection branch 2 times, most recently from d7c0d60 to 599c4f6 Compare February 7, 2023 09:38
src/BaseHtmlElement.php Outdated Show resolved Hide resolved
src/FormElement/Fieldset.php Outdated Show resolved Hide resolved
@nilmerg nilmerg removed this from the v0.7.0 milestone Feb 8, 2023
@TAINCER TAINCER requested review from nilmerg and lippserd February 9, 2023 10:29
@TAINCER TAINCER force-pushed the Collection branch 3 times, most recently from fbac7d9 to b54c353 Compare February 13, 2023 16:54
@TAINCER TAINCER changed the base branch from fix-cloning to master February 14, 2023 10:23
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

Please rebase with master and add proper class and method PHPDoc. Also example usage for the element would be nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants