Skip to content

Add option to display label for checkbox/switch above the field. - #1436

Open
mjauvin wants to merge 4 commits into
developfrom
label-placement
Open

mjauvin wants to merge 4 commits into
developfrom
label-placement

Conversation

@mjauvin

@mjauvin mjauvin commented Jan 8, 2026 •

Copy link
Copy Markdown
Member
Screenshot_20260108-145505 Chrome

Summary by CodeRabbit

  • New Features

    • Added per-field label visibility settings for checkbox, switch, and section fields.
  • Improvements

    • Checkbox and switch field labels now follow their configured visibility, with labels hidden by default when no setting is specified.
    • Help text visibility follows the label setting for checkbox and switch fields.
  • Style

    • Required-field markers are no longer shown on custom-checkbox labels when labels are displayed.

@mjauvin mjauvin self-assigned this Jan 8, 2026
@mjauvin mjauvin added the enhancement PRs that implement a new feature or substantial change label Jan 8, 2026
@coderabbitai

coderabbitai Bot commented Jan 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9f8d36a0-19dd-4cb0-bc89-31207aa6c3ec

📥 Commits

Reviewing files that changed from the base of the PR and between b42b18d and c5a6e0f.


📒 Files selected for processing (2)
  • modules/backend/widgets/Form.php
  • modules/system/assets/ui/less/form.less

🚧 Files skipped from review as they are similar to previous changes (1)
  • modules/system/assets/ui/less/form.less

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.



Walkthrough

A new showLabels property on FormField can be set through field configuration. For checkbox, switch, and section fields, the form widget uses this setting to determine label visibility. The checkbox and switch partials update label and help-text rendering. The stylesheet excludes labels with the show-labels class from required-marker styling. The JavaScript bundles also contain module reordering and callback syntax changes.

Cohort / File(s) Summary
Field configuration
modules/backend/classes/FormField.php
Adds the public showLabels property and applies it through evalConfig.
Form widget and rendering
modules/backend/widgets/Form.php, modules/backend/widgets/form/partials/_field_checkbox.php, modules/backend/widgets/form/partials/_field_switch.php
Uses showLabels for checkbox, switch, and section fields. Updates checkbox and switch label and help-text rendering conditions.
Required-marker styling
modules/system/assets/ui/less/form.less
Excludes labels with the show-labels class from the required-marker selector.
JavaScript bundles
modules/system/assets/js/build/*.js, modules/system/assets/js/snowboard/build/*.js
Includes bundle module reordering and callback syntax changes. The supplied summaries report no observable behavior or public API changes.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to c5a6e

The visible required-field label retains its required marker. No concrete merge-blocking issue is established in the supplied changes.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 7.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 9 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: adding an option to display checkbox and switch labels above the field.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 7.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 9 files. (1 skipped: 1 unsupported.)


  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR


🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 showLabels is true, there are effectively two <label> elements associated with the same input:

  1. A label with text content rendered above the field by the form widget
  2. An empty label element here (with show-labels class)

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8a7f74b and b1fbb6b.

📒 Files selected for processing (6)
  • modules/backend/classes/FormField.php
  • modules/backend/widgets/Form.php
  • modules/backend/widgets/form/partials/_field_checkbox.php
  • modules/backend/widgets/form/partials/_field_switch.php
  • modules/system/assets/ui/less/form.less
  • modules/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 showLabels to the evalConfig array 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 when showLabels is 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; to return $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 showLabels is false. When showLabels is true, the main form widget partial (_field.php) takes over and uses the commentPosition property to render help text either above or below the field, ensuring help text is never lost.

@mjauvin
mjauvin requested a review from LukeTowers January 8, 2026 20:00
@mjauvin mjauvin changed the title Changes to permit displaying label for checkbox/switch above the field Changes to display label for checkbox/switch above the field. Jan 10, 2026
@mjauvin mjauvin changed the title Changes to display label for checkbox/switch above the field. Add option to display label for checkbox/switch above the field. Mar 10, 2026
@mjauvin

mjauvin commented Mar 10, 2026

Copy link
Copy Markdown
Member Author

Note: I bear no responsibility for all those js assets that got recompiled aven though I did not make any change to their dependencies.

@mjauvin mjauvin added this to the 1.2.13 milestone Mar 13, 2026
@LukeTowers LukeTowers modified the milestones: 1.2.13, 1.3.0 Jun 10, 2026
@zimudec

zimudec commented Oct 10, 2026 •

Copy link
Copy Markdown

@mjauvin I verified this on the 1.3.0 line so there's one less unknown for the milestone: as authored this targets develop (Laravel 9), and since the milestone tree has diverged I merged label-placement onto the current wip/1.3 tip (9a2fe625) — one conflict, only in the committed modules/system/assets/ui/storm.css build output, resolved with the PR's side — and ran the checks below there. The findings apply to either branch; leaving the develop-base flow to the maintainers as usual.

What the feature does (verified states). showLabels is now a per-field opt-in for checkbox, switch and section fields (Form.php#showFieldLabels returns $field->showLabels ?? false instead of an unconditional false):

  • Without the config (every current form): byte-for-byte the previous behavior — checkbox/switch keep their inline label and nested help-block comment. The only serialization artifact is an empty class="" on the checkbox's inline label (new placeholder attribute, no effect).
  • With showLabels: true: the field container renders the label above (via _field.php), the checkbox renders its inner label as an empty <label for=… class="show-labels"></label> (association kept, no duplicated text), and for switch the inline label is simply omitted. Runtime-rendered literal HTML for both states confirmed this, and I visually confirmed the green state in the browser at mobile width (390px — the compact-layout case from the screenshot in this PR): the real backend settings form serves the PR's CSS, and I injected the runtime-rendered showLabels markup into it to view the label-above rendering (no throwaway form files were created).

Accessible-name check (WCAG 2.2). With the PR's CSS active I verified in the browser that the label[for] ↔ input association is intact in both states (1.3.1), that the name is visible rather than aria-only (3.3.2), and that the visible label text equals the accessible name (2.5.3). One standards note, deliberately close to @mjauvin's intent and worth stating for the record: G162's layout convention places checkbox labels after the control; this feature deliberately deviates from that convention for showLabels fields (label above) — which is fine, since G162 is one sufficient technique for 3.3.2 and not a conformance requirement — but the PR description may want a sentence acknowledging the opt-in exists to cover the compact-layout case rather than as a general recommendation. Also nice: the LESS fix anchors the required-asterisk to the non-empty label (> .custom-checkbox > label:not(.show-labels):after), so required checkboxes don't render a floating asterisk next to an empty label element.

One semantic question. Under showLabels: true the partial skips its nested comment (help-block inside the checkbox/switch markup), but _field.php still renders the comment at the container level (above/below per commentPosition) — verified both states. Was dropping the nested comment block intended (container-level comment being the replacement), or should a showLabels field with a comment double-guard against the container-level render? As it stands a comment survives in both modes, just in a different position — I'd suggest this is intended but worth a line in the docs PR either way.

Build artifacts churn (non-functional, but worth flagging). 9 of the 14 changed files in this PR are committed build outputs whose diffs are unrelated to label placement: the committed system.js / system.debug.js / snowboard/build/snowboard.* artifacts were re-emitted with a different toolchain revision — this rewrites the Snowboard base module emit (the class … {constructor(e){this.snowboard=e}} pattern becomes an IIFE Proxy(new class extends…) with re-scoped exports; ~60–190 byte growth per file; manifest.js adds two wrapper parentheses, +4 bytes) and storm.css (−15,419 bytes) mixes the label-placement LESS delta with compiler output differences. Since none of this affects behavior (the tests and the browser runs confirm), I'd suggest re-running the build with the repo's current toolchain so the committed artifacts change only where the feature actually needs them — otherwise every future develop→wip/1.3 sync re-emits these files again. No locale files are touched by this PR.

Test gate. modules/backend tests on the merged tree: 301 tests, 718 assertions, 0 failures. modules/system tests: 348 tests / 1,780 assertions with 25 failures — all pre-existing environment failures of the sandbox I ran this in (console/asset Mix/Npm/Vite families that need an npm toolchain the environment doesn't have); I re-ran the same suite on the un-merged tip and it produces the identical 25 failures, so none are attributable to this PR.

@mjauvin

mjauvin commented Oct 10, 2026

Copy link
Copy Markdown
Member Author

Still need to compile core assets (storm.css)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement PRs that implement a new feature or substantial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants