Port FormGroup to Bootstrap 5#38
Merged
Merged
Conversation
Replace the stale Bootstrap 4 carry-over with a proper Bootstrap 5 widget: .mb-3 wrapper, .form-label label and .form-text helper text. Restore the flexible ($label, $content, $placeholder, $helptext, $addTagClass) signature used across callers. The previous strict 3-arg, Input-typed constructor threw on label-only/no-arg calls, non-Input content (ATag, SelectTag) and the 4-arg helptext form, and referenced a non-existent Ease\TWB5\DivTag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 47 minutes and 32 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesFormGroup Constructor Redesign
Navbar Offcanvas Integration
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Ease/TWB5/FormGroup.php`:
- Around line 54-64: The current FormGroup logic generates deterministic IDs
from $label via \Ease\Functions::lettersOnly which can produce duplicate DOM ids
and always emits a LabelTag "for" target even when there is no bindable control;
update the code in FormGroup (references: $id, $label, $content,
\Ease\Functions::lettersOnly, \Ease\Functions::randomString, addItem(),
\Ease\Html\LabelTag) to (1) ensure generated ids are unique by appending a short
random suffix when the id is derived from the label (e.g. $id =
lettersOnly($label) . '_' . randomString(6)), and (2) only set the LabelTag
"for" attribute when there is an actual bindable control in $content (detectable
by is_object($content) and either a public id property or a getter like
getId()/getAttribute('id')); if no bindable control exists, emit the label
without a "for" attribute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d704d00-206f-4929-b74a-21b0261aeb00
📒 Files selected for processing (1)
src/Ease/TWB5/FormGroup.php
Comment on lines
+54
to
+64
| if (empty($id) && \is_string($label) && $label !== '') { | ||
| $id = \Ease\Functions::lettersOnly($label); | ||
| } | ||
|
|
||
| if (empty($id)) { | ||
| $id = 'formgroup_'.\Ease\Functions::randomString(); | ||
| } | ||
|
|
||
| if ($label !== null && $label !== '') { | ||
| $this->addItem(new \Ease\Html\LabelTag((string) $id, $label, ['class' => 'form-label'])); | ||
| } |
There was a problem hiding this comment.
Ensure control IDs are unique and only referenced when a bindable control exists.
Line 54 creates deterministic IDs from label text, so repeated labels can produce duplicate DOM IDs. Also, Line 63 always emits a for target even when $content is non-object/label-only, which creates broken label associations.
💡 Suggested fix
- if (empty($id) && \is_string($label) && $label !== '') {
- $id = \Ease\Functions::lettersOnly($label);
- }
+ if (empty($id) && \is_string($label) && $label !== '') {
+ $id = 'formgroup_'.\Ease\Functions::lettersOnly($label).'_'.\Ease\Functions::randomString();
+ }
if (empty($id)) {
$id = 'formgroup_'.\Ease\Functions::randomString();
}
- if ($label !== null && $label !== '') {
- $this->addItem(new \Ease\Html\LabelTag((string) $id, $label, ['class' => 'form-label']));
- }
+ $canBindLabel = \is_object($content)
+ && (method_exists($content, 'setTagID') || method_exists($content, 'getTagID'));
+ if ($label !== null && $label !== '') {
+ $this->addItem(new \Ease\Html\LabelTag($canBindLabel ? (string) $id : null, $label, ['class' => 'form-label']));
+ }Also applies to: 75-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Ease/TWB5/FormGroup.php` around lines 54 - 64, The current FormGroup
logic generates deterministic IDs from $label via \Ease\Functions::lettersOnly
which can produce duplicate DOM ids and always emits a LabelTag "for" target
even when there is no bindable control; update the code in FormGroup
(references: $id, $label, $content, \Ease\Functions::lettersOnly,
\Ease\Functions::randomString, addItem(), \Ease\Html\LabelTag) to (1) ensure
generated ids are unique by appending a short random suffix when the id is
derived from the label (e.g. $id = lettersOnly($label) . '_' . randomString(6)),
and (2) only set the LabelTag "for" attribute when there is an actual bindable
control in $content (detectable by is_object($content) and either a public id
property or a getter like getId()/getAttribute('id')); if no bindable control
exists, emit the label without a "for" attribute.
OffCanvas no longer renders permanently visible (drop the hardcoded `show` class), accepts a placement argument (start|end|top|bottom), exposes public $header/$body, and gains a triggerButton() helper that mirrors Modal. Navbar gains an opt-in $offcanvas mode that turns the toggler into an offcanvas trigger and wraps the menu in a drawer (responsive offcanvas-in-navbar); the default collapse behaviour is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Ease/TWB5/OffCanvas.php`:
- Around line 57-60: The aria-label for the close ButtonTag is hard-coded to
'Close' — update the OffCanvas header construction to use the project's
translation helper (e.g., t(), __(), or the app's i18n function) instead of the
literal string so the label is localized; modify the ButtonTag attribute
'aria-label' to call the translation helper and ensure the OffCanvas class
(where $this->header is built) uses the correct translation function and
namespace so translated text is rendered at runtime.
- Around line 50-60: In the OffCanvas constructor, avoid creating an empty
accessible name when $title is null: only add the 'aria-labelledby' attribute to
parent::__construct(...) when $title is non-null/non-empty, and only create the
H5Tag($title, ...) (the heading node with id $id.'Label') when $title is
provided; if $title is omitted, still render the close ButtonTag but do not
render an empty H5Tag and do not set aria-labelledby so any caller-supplied
aria-label is preserved (adjust the $this->header construction and the
properties passed to parent::__construct accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 980485d9-0a33-4dd5-9278-887f6994aad9
📒 Files selected for processing (2)
src/Ease/TWB5/Navbar.phpsrc/Ease/TWB5/OffCanvas.php
Comment on lines
+50
to
+60
| parent::__construct(null, array_merge([ | ||
| 'tabindex' => '-1', | ||
| 'id' => $id, | ||
| 'aria-labelledby' => $id.'Label', | ||
| ], $properties)); | ||
| $this->addTagClass('offcanvas offcanvas-'.$placement); | ||
| $this->setTagID($id); | ||
|
|
||
| $this->header = new DivTag([ | ||
| new H5Tag($title, ['class' => 'offcanvas-title', 'id' => $id.'Label']), | ||
| new ButtonTag('', ['class' => 'btn-close', 'data-bs-dismiss' => 'offcanvas', 'aria-label' => 'Close']), | ||
| ], ['class' => 'offcanvas-header']); |
There was a problem hiding this comment.
Preserve an accessible name when $title is omitted.
$title is nullable, but this constructor always sets aria-labelledby and always renders the heading node. When callers pass null, the offcanvas ends up labelled by an empty element, which leaves it unnamed for assistive tech and can override a caller-supplied aria-label.
Suggested fix
public function __construct(
string $id,
$title = null,
$bodyContent = null,
string $placement = 'start',
array $properties = []
) {
- parent::__construct(null, array_merge([
- 'tabindex' => '-1',
- 'aria-labelledby' => $id.'Label',
- ], $properties));
+ $defaultProperties = ['tabindex' => '-1'];
+
+ if ($title !== null) {
+ $defaultProperties['aria-labelledby'] = $id.'Label';
+ } elseif (!isset($properties['aria-label'], $properties['aria-labelledby'])) {
+ $defaultProperties['aria-label'] = 'Offcanvas';
+ }
+
+ parent::__construct(null, array_merge($defaultProperties, $properties));
$this->addTagClass('offcanvas offcanvas-'.$placement);
$this->setTagID($id);
- $this->header = new DivTag([
- new H5Tag($title, ['class' => 'offcanvas-title', 'id' => $id.'Label']),
- new ButtonTag('', ['class' => 'btn-close', 'data-bs-dismiss' => 'offcanvas', 'aria-label' => 'Close']),
- ], ['class' => 'offcanvas-header']);
+ $headerItems = [];
+
+ if ($title !== null) {
+ $headerItems[] = new H5Tag($title, ['class' => 'offcanvas-title', 'id' => $id.'Label']);
+ }
+
+ $headerItems[] = new ButtonTag('', ['class' => 'btn-close', 'data-bs-dismiss' => 'offcanvas', 'aria-label' => 'Close']);
+
+ $this->header = new DivTag($headerItems, ['class' => 'offcanvas-header']);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Ease/TWB5/OffCanvas.php` around lines 50 - 60, In the OffCanvas
constructor, avoid creating an empty accessible name when $title is null: only
add the 'aria-labelledby' attribute to parent::__construct(...) when $title is
non-null/non-empty, and only create the H5Tag($title, ...) (the heading node
with id $id.'Label') when $title is provided; if $title is omitted, still render
the close ButtonTag but do not render an empty H5Tag and do not set
aria-labelledby so any caller-supplied aria-label is preserved (adjust the
$this->header construction and the properties passed to parent::__construct
accordingly).
Comment on lines
+57
to
+60
| $this->header = new DivTag([ | ||
| new H5Tag($title, ['class' => 'offcanvas-title', 'id' => $id.'Label']), | ||
| new ButtonTag('', ['class' => 'btn-close', 'data-bs-dismiss' => 'offcanvas', 'aria-label' => 'Close']), | ||
| ], ['class' => 'offcanvas-header']); |
There was a problem hiding this comment.
Localize the close button label.
aria-label => 'Close' hard-codes English in a reusable widget. That leaks untranslated UI text into every locale using this component.
Suggested fix
- new ButtonTag('', ['class' => 'btn-close', 'data-bs-dismiss' => 'offcanvas', 'aria-label' => 'Close']),
+ new ButtonTag('', ['class' => 'btn-close', 'data-bs-dismiss' => 'offcanvas', 'aria-label' => _('Close')]),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Ease/TWB5/OffCanvas.php` around lines 57 - 60, The aria-label for the
close ButtonTag is hard-coded to 'Close' — update the OffCanvas header
construction to use the project's translation helper (e.g., t(), __(), or the
app's i18n function) instead of the literal string so the label is localized;
modify the ButtonTag attribute 'aria-label' to call the translation helper and
ensure the OffCanvas class (where $this->header is built) uses the correct
translation function and namespace so translated text is rendered at runtime.
composer update --lock only refreshes the hash; actually re-resolve so the locked set is installable (composer-normalize ^2.51 pulls justinrainbow/json-schema 6.9.0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
Ease\TWB5\FormGroupwidget was a stale carry-over from the Bootstrap 4 package and broke many callers.Changes
.mb-3wrapper,.form-labellabel,.form-texthelper text.($label, $content, $placeholder, $helptext, $addTagClass)signature. The previous strict 3-arg,Input-typed constructor threw on label-only/no-arg calls, non-Inputcontent (ATag,SelectTag), the 4-arg helptext form, and referenced a non-existentEase\TWB5\DivTag.Testing
Rendered the widget for textarea+helptext, 2/3-arg,
ATag, no-arg and label-only cases, and smoke-tested 60+ call sites in the consuming app (login, profile, credential-type, event-source forms) — all render with correct BS5 markup and no errors.🤖 Generated with Claude Code
Summary by CodeRabbit