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

Allow configuration of checkbox ActiveField label class name #203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

antichris
Copy link

For no apparent reason (not apparent to me, that is) the author of ActiveField decided, that the class attribute of a checkbox label should not be configurable. I beg to differ.

Q A
Is bugfix? yes/no
New feature? no/yes
Breaks BC? possibly
Tests pass? yes
Fixed issues no issue opened (yet)

BC could get broken for those relying on class being ignored in labelOptions (no idea why would anyone want to do that, though).

I figured that opening an issue for discussion would take more time then just creating a PR that fixes the problem, since an existing pull request can be merged or rejected any time, while an issue would require someone to create a PR for it to be resolved anyway.

Also add test cases verifying this functionality. And the missing
trailing newline at the end of the test file.
@samdark samdark added this to the 2.0.7 milestone May 31, 2017
@samdark samdark added the status:to be verified Needs to be reproduced and validated. label May 31, 2017
@samdark
Copy link
Member

samdark commented May 31, 2017

Since it's there from the very beginning mikehaertl/yii2@7364249#diff-37ce2b0674ccb8129c6a33875aec0bbfR177. @mikehaertl do you remember why the class is nulled in case of checkboxes and radios?

@mikehaertl
Copy link
Contributor

The examples in the BS3 docs have no class set for checkbox labels. But if you don't null it, you inherit the default class control-label from yii\widgets\ActiveField, which could possibly result in unexpected effects.

But I agree that there's a problem if you want to explicitely set a class. I'm just not sure about a proper fix.

@antichris
Copy link
Author

I am well aware that this can actually be worked around by passing the class name(s) in the config parameter for ActiveField::label(), but that's a workaround and I strongly believe in getting things done without workarounds.

@samdark samdark modified the milestones: 2.0.7, 2.0.8 Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:to be verified Needs to be reproduced and validated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants