-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
Enhance label()
method in ActiveField
with tag
option and fix labelOptions
for checkbox
/radio
(#20537).
#20546
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
base: 22.0
Are you sure you want to change the base?
Conversation
… with `enclosedByLabel = false` in `checkbox`/`radio` methods (yiisoft#20537).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 22.0 #20546 +/- ##
============================================
- Coverage 65.61% 64.50% -1.11%
- Complexity 11263 11267 +4
============================================
Files 428 428
Lines 37068 37083 +15
============================================
- Hits 24323 23922 -401
- Misses 12745 13161 +416 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
maybe can reduce redundant code ? so it's more easy to maintain later |
Yes it is a good idea. |
I'm thinking,
|
If it's a good idea, i'll do it in another PR. |
labelOptions
are ignored when using custom label with enclosedByLabel = false
in checkbox
/radio
methods (#20537).label()
method with tag
option and fix labelOptions
for checkbox
/radio
(#20537).
label()
method with tag
option and fix labelOptions
for checkbox
/radio
(#20537).label()
method with tag
option and fix labelOptions
for checkbox
/radio
(#20537).
… and improve label rendering logic.
done. |
label()
method with tag
option and fix labelOptions
for checkbox
/radio
(#20537).label()
method in ActiveField
with tag
option and fix labelOptions
for checkbox
/radio
(#20537).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the ActiveField::label()
method with a new tag
option to control the wrapper element type and fixes how labelOptions
work for checkbox/radio inputs when not enclosed by label. The enhancement maintains full backward compatibility while providing more flexibility for custom label rendering.
Key Changes:
- Added
tag
option tolabel()
method for customizing wrapper elements - Fixed
labelOptions
handling in checkbox/radio methods whenenclosedByLabel
is false - Added comprehensive test coverage for new functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
framework/widgets/ActiveField.php |
Core implementation of tag option in label() method and refactored checkbox/radio label handling |
tests/framework/widgets/ActiveFieldTest.php |
Comprehensive test coverage for new label functionality and improved test format |
framework/UPGRADE.md |
Documentation of new feature for upgrade guide |
framework/CHANGELOG.md |
Added entry for enhancement #20537 |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
framework/widgets/ActiveField.php
Outdated
public function label($label = null, $options = []) | ||
{ | ||
if ($label === false) { | ||
if ($label === false || (isset($this->labelOptions['label']) && $this->labelOptions['label'] === false)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition check for $this->labelOptions['label'] === false
seems unexpected here. The labelOptions
array is typically for HTML attributes, not for controlling label visibility. This could create confusion and should be documented or reconsidered.
if ($label === false || (isset($this->labelOptions['label']) && $this->labelOptions['label'] === false)) { | |
if ($label === false) { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think user should allowed to overwrite the label with newer value, isn't it ?
If set to `false`, the label content will be rendered as raw HTML without any wrapper tag.
except it's null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the user can overwrite the label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean
$this->activeField(['label'=>false])->label('anything');
newer should be the final decision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean
$this->activeField(['label'=>false])->label('anything');
newer should be the final decision
@samdark How would you treat this edge case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Label precedence.
$label
parameter (highest priority).options['label']
(only when$label
===null
).Model attribute label (fallback)
.- If any of these equals
false
, the label won't render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
framework/widgets/ActiveField.php
Outdated
public function label($label = null, $options = []) | ||
{ | ||
if ($label === false) { | ||
if ($label === false || (isset($this->labelOptions['label']) && $this->labelOptions['label'] === false)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean
$this->activeField(['label'=>false])->label('anything');
newer should be the final decision
$this->activeField(['label'=>false])->label('anything'); done Add more tests. thks, for review 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
*/ | ||
protected function generateLabel(array $options): array | ||
{ | ||
if (isset($options['label']) && $options['label'] !== false && $this->parts['{label}'] === '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider extracting the condition $this->parts['{label}'] === ''
into a more descriptive variable or method name to improve readability, such as $isLabelEmpty
or $this->isLabelEmpty()
.
Copilot uses AI. Check for mistakes.
|
||
unset($options['labelOptions']['tag']); | ||
|
||
$this->labelOptions = array_merge($this->labelOptions, $options['labelOptions']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The labelOptions
merging could overwrite existing options unintentionally. Consider using a more explicit merge strategy or documenting the precedence behavior to avoid unexpected side effects.
Copilot uses AI. Check for mistakes.
Previous Behavior:
label()
generates a standard<label>
tag with the model attribute label.label('Custom Text')
generates a<label>
tag with custom text.label(false)
removes the label entirely from the field (empty string).label($text, $options)
generates a label with custom options merged withlabelOptions
.New Behavior:
tag
=>'label'
default usesHtml::activeLabel()
to generate a standard<label>
element with the for attribute.tag
=>false
renders the label content as raw HTML without any wrapper tag.tag
=>span
/div
/h3
/etc - usesHtml::tag()
to generate the specified element.