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 common interface to ControlGroup and Container #131

Open
richard-ejem opened this issue Aug 11, 2016 · 6 comments
Open

Add common interface to ControlGroup and Container #131

richard-ejem opened this issue Aug 11, 2016 · 6 comments
Milestone

Comments

@richard-ejem
Copy link

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

@jtojnar
Copy link
Contributor

jtojnar commented Sep 4, 2016

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');

@richard-ejem
Copy link
Author

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.

@f3l1x
Copy link
Member

f3l1x commented Jun 18, 2017

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?

@dg
Copy link
Member

dg commented Jun 19, 2017

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

@f3l1x
Copy link
Member

f3l1x commented Jun 19, 2017

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

@dg
Copy link
Member

dg commented Jun 19, 2017

It is not easy.

@dg dg added this to the v3.0 milestone Jun 21, 2017
@dg dg modified the milestones: v3.0, v4.0 Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants