forms icon indicating copy to clipboard operation
forms copied to clipboard

Add common interface to ControlGroup and Container

Open richard-ejem opened this issue 8 years ago • 6 comments

Common interface declaring getControls() would be useful for form renderers including DefaultFormRenderer.

Example:

interface IControlContainer
{
    /** @return IControl[] */
    public function getControls();
}

then, DefaultFormRenderer::renderControls() could look like:

public function renderControls(Nette\Forms\IControlContainer $parent) {
    # got rid of this check:
    # if (!($parent instanceof Nette\Forms\Container || $parent instanceof Nette\Forms\ControlGroup)) {
    #     throw new Nette\InvalidArgumentException('Argument must be Nette\Forms\Container or Nette\Forms\ControlGroup instance.');
    # }
}

If agreed, I can pullrequest it

richard-ejem avatar Aug 11 '16 07:08 richard-ejem

Why not make ControlGroup implement IContainer? It would also allow to have groups inside groups and get rid of the weird setCurrentGroup() switching.

Isn’t the left one just more natural?

$form = new UI\Form;
$g1 = $form->addGroup('Group 1');
$g1->addText('foo', 'Foo');
$g2 = $form->addGroup('Group 2');
$g2->addText('bar', 'Bar');
$form = new UI\Form;
$g1 = $form->addGroup('Group 1');
$form->setCurrentGroup($g1);
$form->addText('foo', 'Foo');
$g2 = $form->addGroup('Group 2');
$form->setCurrentGroup($g2);
$form->addText('bar', 'Bar');

jtojnar avatar Sep 04 '16 14:09 jtojnar

Yes it is, but it is a major API change, maybe for Nette 3 one day :) for now, unifying them using interface would be more likely a simple fix of current state.

richard-ejem avatar Sep 04 '16 15:09 richard-ejem

Maybe it could simplify the whole thing that ControlGroup and Container should merge it together.

ControlGroup is just for form.fieldset? I mean graphic purpose?

f3l1x avatar Jun 18 '17 12:06 f3l1x

ControlGroup is any general group of controls. I think that it is used only for graphic purposes.

dg avatar Jun 19 '17 15:06 dg

What about drop it at all? We could use container for that too?

f3l1x avatar Jun 19 '17 15:06 f3l1x

It is not easy.

dg avatar Jun 19 '17 15:06 dg