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

Inconsistent return type of getLabel #241

Closed
dakujem opened this issue Mar 23, 2020 · 4 comments
Closed

Inconsistent return type of getLabel #241

dakujem opened this issue Mar 23, 2020 · 4 comments

Comments

@dakujem
Copy link

@dakujem dakujem commented Mar 23, 2020

Version: 3.*

Problem description

For some reason, CheckBox, HiddenField and Button have no label by default. But the other inputs do have labels.
image

What If we did want our checkboxes to actually have a label? And render it like a label of the other elements? (For design consistency or any other reason.)
image

Of course I know manual rendering could be used. But why?

Or we wanted to make it Bootstrap-compatible (render the input outside of <label> tag)?

If anyone could make Nette forms use their own implementation of Checkbox ... but we can't, since in order to use addCheckbox method we need to return an instance of Nette\Forms\Control\Checkbox and since its getLabel method is defined as public function getLabel($caption = null): void there is no way to force any other return type on it...

Possible solution

  1. drop void return type on Checkbox::getLabel method.
  2. implement a mechanism to allow users to be able to select how they want the label be rendered.

In Nette 2.4 we have solved this elegantly this way (one is able to chose from 3 different styles of how getControl and getLabel return their values):

    public function getControl()
    {
        $flag = $this->getLabelRenderFlag();

        // label and control wrapped together
        if ($flag === self::LABEL_NETTE_STYLE) {
            return $this->parent_getControl();
        }

        // wrap label and label separately
        if ($flag === self::LABEL_SEPARATE_WRAPPER) {
            $this->getSeparatorPrototype()->addHtml($this->getControlPart());
            $this->getSeparatorPrototype()->addHtml($this->getLabelPart());
            return $this->getSeparatorPrototype();
        }

        // only control part returned (label is rendered separately)
        return $this->getSeparatorPrototype()->addHtml($this->getControlPart());
    }

    public function getLabel($caption = null)
    {
        // label is not rendered
        if ($this->getLabelRenderFlag() !== self::LABEL_STANDALONE) {
            return parent::getLabel($caption);
        }

        // render label separately
        return $this->getLabelPart();
    }

I'm open to providing a PR for the above code.


Please also consider dropping the return type of the other methods, for consistency sake, even though it makes less sense to override labels of these methods:

  • RadioList::getLabel
  • HiddenField::getLabel
  • Button::getLabel
@dg dg closed this as completed in d2d89b0 Apr 20, 2020
@dakujem
Copy link
Author

@dakujem dakujem commented Apr 29, 2020

Awesome! Thanks a lot.

How about RadioList's Html return type?

@dg
Copy link
Member

@dg dg commented Apr 30, 2020

@dakujem
Copy link
Author

@dakujem dakujem commented May 1, 2020

Any return type is covariant to "not specified". 🤷‍♂️

Anyway, it limits extensibility and breaks consistency.

BUT hey, we are at least able to return any string (including empty strings) wrapped inside Html objects, so it's no biggie. 👍
Thanks for the other changes, appreciated.

@dg
Copy link
Member

@dg dg commented May 2, 2020

It is specified

* @return Html|string|null

dakorpar added a commit to contributte/forms-bootstrap that referenced this issue Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants