Add option to display label for checkbox/switch above the field.#1436
Add option to display label for checkbox/switch above the field.#1436
Conversation
mjauvin
added
the
enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Field Config
participant Widget as Form Widget
participant Field as FormField
participant Partial as Template Partial
participant Browser as Browser/CSS
Config->>Widget: provide field config (includes showLabels)
Widget->>Field: evalConfig() -> set showLabels
Widget->>Partial: renderField(field)
Partial->>Field: check field.showLabels
alt showLabels == true
Partial->>Browser: render input without label (add .show-labels class)
Browser->>Browser: CSS hides asterisk for .show-labels
else showLabels == false
Partial->>Browser: render label and help text
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
modules/backend/classes/FormField.php (1)
183-186: Update documentation to include 'section' field type and consider adding type hints.The docblock mentions "checkbox/switch fields" but the implementation in
modules/backend/widgets/Form.php(line 1158) also applies this property to 'section' field types. Additionally, consider adding a type hint and explicit default value for better type safety and clarity.📝 Suggested improvements
/** - * @var bool Should the field's label for checkbox/switch fields be displayed above the field. + * @var bool|null Should the field's label for checkbox/switch/section fields be displayed above the field. + * When true, the label is rendered above the field by the form widget. + * When false or null (default), the label is rendered inline by the field partial. */ -public $showLabels; +public $showLabels = null;modules/backend/widgets/form/partials/_field_checkbox.php (1)
18-22: Consider accessibility implications of dual label elements.When
showLabelsis true, there are effectively two<label>elements associated with the same input:
- A label with text content rendered above the field by the form widget
- An empty label element here (with
show-labelsclass)While this maintains the HTML structure, having two labels for one input may have semantic implications. Consider verifying that screen readers handle this correctly and whether ARIA attributes might improve clarity.
If you'd like to verify the accessibility of this implementation, consider testing with screen readers (NVDA, JAWS, VoiceOver) to ensure the dual-label structure doesn't cause confusion.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
modules/backend/classes/FormField.phpmodules/backend/widgets/Form.phpmodules/backend/widgets/form/partials/_field_checkbox.phpmodules/backend/widgets/form/partials/_field_switch.phpmodules/system/assets/ui/less/form.lessmodules/system/assets/ui/storm.css
🧰 Additional context used
🧬 Code graph analysis (2)
modules/backend/widgets/form/partials/_field_checkbox.php (1)
modules/backend/classes/FormField.php (2)
getId(604-622)comment(361-371)
modules/backend/widgets/form/partials/_field_switch.php (1)
modules/backend/classes/FormField.php (2)
getId(604-622)comment(361-371)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: ubuntu-latest / PHP 8.4
- GitHub Check: ubuntu-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.3
- GitHub Check: windows-latest / JavaScript
🔇 Additional comments (5)
modules/backend/classes/FormField.php (1)
287-302: LGTM: showLabels properly integrated into config processing.The addition of
showLabelsto theevalConfigarray correctly enables this property to be set via field configuration, maintaining consistency with other form field properties.modules/system/assets/ui/less/form.less (1)
125-139: LGTM: CSS correctly excludes required asterisk for labels in label-above mode.The
:not(.show-labels)selector appropriately prevents the required asterisk from appearing on the inline label element whenshowLabelsis true. In this mode, the asterisk appears on the label rendered above by the form widget, and the inline label element is empty and should not display an asterisk.modules/backend/widgets/form/partials/_field_checkbox.php (1)
24-26: LGTM: Help text rendering logic is consistent.The conditional rendering of help text matches the pattern used in the switch partial, correctly showing help text only when labels are rendered inline (showLabels is false).
modules/backend/widgets/Form.php (1)
1156-1167: LGTM: Per-field label visibility control implemented correctly.The change from
return false;toreturn $field->showLabels ?? false;enables per-field configuration while maintaining backward compatibility. The null coalescing operator ensures existing forms continue to work with the default inline label behavior.The implementation correctly:
- Applies to checkbox, switch, and section field types
- Preserves widget field behavior (which has its own showLabels property)
- Defaults to false when showLabels is not explicitly set
- Integrates with the FormField.showLabels property and partial rendering logic
modules/backend/widgets/form/partials/_field_switch.php (1)
14-19: Code logic is correct and properly integrated with the form widget.The conditional rendering in this partial correctly hides the label and help text when
showLabelsis false. WhenshowLabelsis true, the main form widget partial (_field.php) takes over and uses thecommentPositionproperty to render help text either above or below the field, ensuring help text is never lost.
mjauvin
changed the title
mjauvin
changed the title
|
Note: I bear no responsibility for all those js assets that got recompiled aven though I did not make any change to their dependencies. |
1 participant
Summary by CodeRabbit
New Features
showLabelssetting, allowing control over whether labels appear above these field types.Improvements
Style