Added expand/collapse functionalities to relationship formwidget#1374
Added expand/collapse functionalities to relationship formwidget#1374damsfx wants to merge 11 commits intowintercms:developfrom
Conversation
|
@damsfx ooo, I like this. And in Snowboard too! 😍 Couple of quick comments/questions:
|
Definitely yes! Despite this, a function written in jQuery is still called to update the scroll bar. :o(
The tree structure used for my tests and visible in the animation above contains 550 nodes spread over 4 levels of depth.
For sure. |
|
Latest update :
|
|
@damsfx don't be too concerned if you need to fall back on jQuery to do something in the Backend. A lot of the Backend still uses it and the goal of Snowboard was to remove the dependency of jQuery for the CMS side in particular. |
|
@bennothommo are you happy with this as it is? @damsfx is there a related docs PR? |
LukeTowers
added
needs review
Not yet, but I can add a pending PR for this. |
Co-authored-by: Luke Towers <github@luketowers.ca>
@LukeTowers Done : wintercms/docs#243 |
|
@bennothommo I know your time is precious, but if you have 5 mins to take a look and give me a little feedback if you see any improvements to be made. |
|
@damsfx sorry for the delay. Looks great, and the timing of the animation is much better. Only issue I can see now is that it appears that some plugins and themes were added in your most recent commit: Could you make sure these are not included in your branch? Once that's done, @LukeTowers should be able to merge if he's happy with it. |
bennothommo
removed
the
needs review
bennothommo
left a comment
bennothommo
left a comment
There was a problem hiding this comment.
Approved, pending removal of plugins and themes in recent commit
😱🙄 Oh, I was really distracted! Done ! |
|
@LukeTowers @bennothommo Is this still an interesting PR for WinterCMS !? |
|
@damsfx yes, I had completely forgotten about this. Can you resolve the comments and ping me when it's ready to review? |
LukeTowers
added
needs review
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces a tree-structured relation form widget with expand/collapse functionality. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/backend/formwidgets/Relation.php (1)
184-196:⚠️ Potential issue | 🟠 MajorDuplicate
$field->optionsassignment — the first one (lines 184–186) is dead code.Lines 184–186 set
$field->options, then lines 188–196 immediately overwrite it with the new tree-aware logic. The original assignment should be removed.Proposed fix
- $field->options = $usesTree - ? $result->listsNested($nameFrom, $primaryKeyName) - : $result->lists($nameFrom, $primaryKeyName); - if ($usesTree) { if ($this->displayTree) { $field->options = $result->toNestedArray($nameFrom, $primaryKeyName); } else { $field->options = $result->listsNested($nameFrom, $primaryKeyName); } } else { $field->options = $result->lists($nameFrom, $primaryKeyName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/formwidgets/Relation.php` around lines 184 - 196, Remove the dead initial assignment to $field->options that uses the ternary based on $usesTree; instead keep the subsequent conditional block which correctly handles $usesTree and $this->displayTree and calls $result->toNestedArray, $result->listsNested, or $result->lists as appropriate. Specifically, delete the first assignment (the ternary setting $field->options) and rely on the existing if ($usesTree) { ... } else { ... } logic in Relation.php so $field->options is only set once with the correct tree-aware method.
🧹 Nitpick comments (7)
modules/backend/formwidgets/relation/assets/less/relation.less (1)
16-19: Preferflex-startoverstartforjustify-content.
justify-content: startis a CSS Box Alignment value, whileflex-startis the original Flexbox-specific value with broader browser support. Given the project's browser targets,flex-startis the safer choice.Proposed fix
- justify-content: start; + justify-content: flex-start;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/formwidgets/relation/assets/less/relation.less` around lines 16 - 19, Replace the CSS value "start" with the Flexbox-specific "flex-start" for the justify-content declaration in relation.less (the block that sets display: flex; flex-direction: row; align-items: center; justify-content: start;), changing justify-content to "flex-start" to ensure broader browser support.modules/backend/lang/en/lang.php (1)
326-328: Minor terminology inconsistency with existing dashboard translations.The
dashboardsection (lines 107–108) uses "Collapse all" / "Expand all", while these newformkeys use "Close all" / "Open all". This isn't a bug, but you might want to unify the wording for a consistent user experience across the backend UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/lang/en/lang.php` around lines 326 - 328, The wording in the form translation keys is inconsistent with the dashboard translations: update the values for 'expand_all', 'collapse_all', and 'expand_checked' in the lang.php translations so they match the dashboard terminology (use "Expand all" instead of "Open all", "Collapse all" instead of "Close all", and "Expand selected" instead of "Open selected") to ensure consistent UX across the backend UI.modules/backend/formwidgets/relation/assets/js/src/Relation.js (3)
183-199: ParameterelinonToggleis actually anEvent, not anHTMLElement.The
addEventListener('click', this.events.toggle)passes theEventobject. The parameter is namedel(and documented as@param {HTMLElement} el), butel.target.parentElementis used, confirming it's an Event. Rename for clarity.Proposed fix
- onToggle(el) { - const tooglePromise = new Promise((resolve, reject) => { - let parent = el.target.parentElement; + onToggle(event) { + const togglePromise = new Promise((resolve, reject) => { + let parent = event.target.parentElement;Also fixes the
tooglePromisetypo →togglePromise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/formwidgets/relation/assets/js/src/Relation.js` around lines 183 - 199, The onToggle method currently treats its parameter as an HTMLElement and names it `el`, but it receives an Event from addEventListener('click', this.events.toggle); rename the parameter (e.g., `event` or `evt`) and update usages to `event.target.parentElement` (or safely `event.currentTarget` if appropriate) and correct the variable typo `tooglePromise` to `togglePromise` in the onToggle function so the code is clearer and accurate; ensure JSDoc/@param is updated from HTMLElement to Event as well.
88-95:querySelectorAllmatches all descendants, not just direct children.
el.querySelectorAll('.checkboxlist-children')[0]will match deeply nested.checkboxlist-childrenin addition to the direct child. Use:scope > .checkboxlist-childrento restrict to direct children and avoid unintended matches in deeply nested trees.Proposed fix for openLevel and closeLevel
openLevel(el) { el.classList.add('open'); - let child = el.querySelectorAll('.checkboxlist-children')[0]; + let child = el.querySelector(':scope > .checkboxlist-children'); if (child) { child.classList.add('open'); } } closeLevel(el) { el.classList.remove('open'); - let child = el.querySelectorAll('.checkboxlist-children')[0]; + let child = el.querySelector(':scope > .checkboxlist-children'); if (child) { child.classList.remove('open'); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/formwidgets/relation/assets/js/src/Relation.js` around lines 88 - 95, openLevel (and the sibling closeLevel) currently use el.querySelectorAll('.checkboxlist-children')[0], which can return deeply nested descendants; change these to select only direct children by using el.querySelector(':scope > .checkboxlist-children') (or an equivalent direct-child selection) so only the immediate .checkboxlist-children element is toggled in openLevel and closeLevel; update both functions (openLevel and closeLevel) to use the :scope > selector and keep the existing classList.add/remove logic.
116-130: Promises add no value here — all operations are synchronous.
onExpandAll,onCollapseAll,onExpandChecked, andonToggleall wrap synchronous DOM class manipulations innew Promise(...). Since the executor runs synchronously, the.then()callback fires in the same microtask batch. Plain sequential calls would be clearer.Example simplification for onExpandAll
onExpandAll() { - const openPromise = new Promise((resolve, reject) => { - let animatedNodes = this.getExpandableNodes(); - - animatedNodes.forEach((item) => { - this.openLevel(item); - }); - - resolve([].slice.call(animatedNodes).pop()); - }); - - openPromise.then((el) => { - this.updateScollBar(el); - }); + const animatedNodes = this.getExpandableNodes(); + animatedNodes.forEach((item) => this.openLevel(item)); + this.updateScrollBar(animatedNodes[animatedNodes.length - 1]); }Same pattern applies to
onCollapseAll,onExpandChecked, andonToggle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/formwidgets/relation/assets/js/src/Relation.js` around lines 116 - 130, The Promise wrapper is unnecessary because getExpandableNodes(), openLevel(), etc. are synchronous; in onExpandAll remove the new Promise(...) and simply iterate over this.getExpandableNodes(), call this.openLevel(item) for each, then call this.updateScollBar(...) directly with the last element; apply the same simplification to onCollapseAll, onExpandChecked, and onToggle (remove new Promise, perform DOM class changes synchronously, then call the corresponding this.updateScollBar/updateScrollBar invocation). Use the existing method names getExpandableNodes, openLevel, onCollapseAll, onExpandChecked, onToggle, and updateScollBar when locating code to change.modules/backend/formwidgets/Relation.php (2)
52-55: Minor docblock typo."rendered has a displayTree" → "rendered as a tree".
Proposed fix
/** - * `@var` bool Define if the widget must be rendered has a displayTree. + * `@var` bool Define if the widget must be rendered as a tree. */ public $displayTree;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/formwidgets/Relation.php` around lines 52 - 55, Docblock for the Relation widget property $displayTree contains a typo ("rendered has a displayTree"); update the comment for the public $displayTree property in Relation.php to read clearly (e.g., "Define if the widget must be rendered as a tree") so the docblock accurately describes the boolean flag and its purpose.
163-164:class_uses()doesn't traverse parent classes — useclass_uses_recursive()instead.
class_uses($relationModel)only returns traits directly used by the class, not inherited ones. If a model extends a base that usesNestedTree, this check will silently fail anddisplayTreewill have no effect. This is pre-existing code, but the newdisplayTreefeature now depends on it. The codebase already usesclass_uses_recursive()elsewhere (InspectableContainer.php), establishing the correct pattern.Proposed fix
- $usesTree = count(array_intersect($treeTraits, class_uses($relationModel))) > 0; + $usesTree = count(array_intersect($treeTraits, class_uses_recursive($relationModel))) > 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/backend/formwidgets/Relation.php` around lines 163 - 164, The check that determines whether the relation model uses a tree trait uses class_uses($relationModel), which only returns traits declared on the class itself; replace that call with class_uses_recursive($relationModel) so inherited traits like Winter\Storm\Database\Traits\NestedTree or SimpleTree are detected; update the $usesTree assignment (which compares $treeTraits against the class_uses result) to call class_uses_recursive($relationModel) instead of class_uses($relationModel) so displayTree behavior works for models inheriting tree traits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/backend/formwidgets/Relation.php`:
- Around line 188-190: The code calls a non-existent method toNestedArray on the
collection ($result->toNestedArray($nameFrom, $primaryKeyName)) when
$this->displayTree is true; replace that call with the existing, tested
collection helper listsNested (i.e. use $result->listsNested($nameFrom,
$primaryKeyName)) or implement an equivalent toNestedArray on the collection,
and keep the fallback logic for non-tree relations intact so Relation.php uses
listsNested instead of toNestedArray when building $field->options.
In `@modules/backend/formwidgets/relation/assets/js/src/Relation.js`:
- Around line 73-81: destruct() currently assumes the tree action controls exist
and calls removeEventListener unconditionally, causing a TypeError when
expandAllControl/collapseAllControl/expandCheckedControl are null; update
destruct() to mirror attachEvents() by null-checking each of expandAllControl,
collapseAllControl, and expandCheckedControl before calling removeEventListener,
and keep the existing loop for this.toggles that removes events.toggle;
reference the destruct() and attachEvents() methods and the properties
expandAllControl, collapseAllControl, expandCheckedControl, toggles, and
events.toggle when making the change.
- Around line 206-216: The method updateScollBar has a typo and can throw a
TypeError when openedLevel is null; rename the method to updateScrollBar and
update every call site that uses updateScollBar, then inside the (now)
updateScrollBar method compute openedLevel as before but guard it with a null
check (if openedLevel is null/undefined return) before calling
openedLevel.addEventListener; keep the existing transitionend handler and
options unchanged. Ensure all references in this file that called updateScollBar
(the earlier calls around the checkbox/children handling) are renamed to
updateScrollBar so usages and exports stay consistent.
In `@modules/backend/formwidgets/relation/assets/less/relation.less`:
- Around line 4-9: Remove the empty line between the rule opening brace and the
comment in relation.less: locate the selector ".field-checkboxlist
.checkboxlist-controls > div:nth-child(even)" and the comment "// Top widget
controls" and place the comment immediately after the opening brace (no blank
line) so the scss/double-slash-comment-empty-line-before lint rule is satisfied.
In `@modules/backend/widgets/form/partials/_field_checkboxlist.php`:
- Line 136: The call to e(renderCheckboxLine(...)) is incorrect because
renderCheckboxLine() echoes HTML and returns void; remove the unnecessary e()
wrapper and call renderCheckboxLine($field, $checkedValues, $children,
$nameFrom, $readOnly, $index) directly in the template so the output is rendered
as intended, and if any escaping is actually required wrap the producing
function to return a string instead of echoing before escaping.
- Around line 96-143: The named function renderCheckboxLine causes a fatal
redeclaration when the partial is included multiple times; replace it with a
recursive closure: define $renderCheckboxLine = function($field, array
$checkedValues, array $fieldOptions, string $nameFrom, bool $readOnly, int
&$index) use (&$renderCheckboxLine) { ... } containing the same body, and update
the recursive call and any invocations to call $renderCheckboxLine(...) (remove
the wrapping e(...) used for renderCheckboxLine since the closure echoes
directly) so the template uses the closure instead of a global function.
- Line 2: The partial currently unconditionally reads $this->nameFrom which
doesn't exist on Form widget; change the assignment so it only reads
$this->nameFrom when available and otherwise uses a passed-in fallback (e.g. a
local $nameFrom variable) or null — do not access $this->nameFrom on Form
contexts. Ensure the conditional access is applied before any use (notably where
displayTree is handled around the block that references nameFrom later), and
update the partial to accept/use a provided $nameFrom when rendered from widgets
that don't expose $this->nameFrom.
---
Outside diff comments:
In `@modules/backend/formwidgets/Relation.php`:
- Around line 184-196: Remove the dead initial assignment to $field->options
that uses the ternary based on $usesTree; instead keep the subsequent
conditional block which correctly handles $usesTree and $this->displayTree and
calls $result->toNestedArray, $result->listsNested, or $result->lists as
appropriate. Specifically, delete the first assignment (the ternary setting
$field->options) and rely on the existing if ($usesTree) { ... } else { ... }
logic in Relation.php so $field->options is only set once with the correct
tree-aware method.
---
Nitpick comments:
In `@modules/backend/formwidgets/Relation.php`:
- Around line 52-55: Docblock for the Relation widget property $displayTree
contains a typo ("rendered has a displayTree"); update the comment for the
public $displayTree property in Relation.php to read clearly (e.g., "Define if
the widget must be rendered as a tree") so the docblock accurately describes the
boolean flag and its purpose.
- Around line 163-164: The check that determines whether the relation model uses
a tree trait uses class_uses($relationModel), which only returns traits declared
on the class itself; replace that call with class_uses_recursive($relationModel)
so inherited traits like Winter\Storm\Database\Traits\NestedTree or SimpleTree
are detected; update the $usesTree assignment (which compares $treeTraits
against the class_uses result) to call class_uses_recursive($relationModel)
instead of class_uses($relationModel) so displayTree behavior works for models
inheriting tree traits.
In `@modules/backend/formwidgets/relation/assets/js/src/Relation.js`:
- Around line 183-199: The onToggle method currently treats its parameter as an
HTMLElement and names it `el`, but it receives an Event from
addEventListener('click', this.events.toggle); rename the parameter (e.g.,
`event` or `evt`) and update usages to `event.target.parentElement` (or safely
`event.currentTarget` if appropriate) and correct the variable typo
`tooglePromise` to `togglePromise` in the onToggle function so the code is
clearer and accurate; ensure JSDoc/@param is updated from HTMLElement to Event
as well.
- Around line 88-95: openLevel (and the sibling closeLevel) currently use
el.querySelectorAll('.checkboxlist-children')[0], which can return deeply nested
descendants; change these to select only direct children by using
el.querySelector(':scope > .checkboxlist-children') (or an equivalent
direct-child selection) so only the immediate .checkboxlist-children element is
toggled in openLevel and closeLevel; update both functions (openLevel and
closeLevel) to use the :scope > selector and keep the existing
classList.add/remove logic.
- Around line 116-130: The Promise wrapper is unnecessary because
getExpandableNodes(), openLevel(), etc. are synchronous; in onExpandAll remove
the new Promise(...) and simply iterate over this.getExpandableNodes(), call
this.openLevel(item) for each, then call this.updateScollBar(...) directly with
the last element; apply the same simplification to onCollapseAll,
onExpandChecked, and onToggle (remove new Promise, perform DOM class changes
synchronously, then call the corresponding this.updateScollBar/updateScrollBar
invocation). Use the existing method names getExpandableNodes, openLevel,
onCollapseAll, onExpandChecked, onToggle, and updateScollBar when locating code
to change.
In `@modules/backend/formwidgets/relation/assets/less/relation.less`:
- Around line 16-19: Replace the CSS value "start" with the Flexbox-specific
"flex-start" for the justify-content declaration in relation.less (the block
that sets display: flex; flex-direction: row; align-items: center;
justify-content: start;), changing justify-content to "flex-start" to ensure
broader browser support.
In `@modules/backend/lang/en/lang.php`:
- Around line 326-328: The wording in the form translation keys is inconsistent
with the dashboard translations: update the values for 'expand_all',
'collapse_all', and 'expand_checked' in the lang.php translations so they match
the dashboard terminology (use "Expand all" instead of "Open all", "Collapse
all" instead of "Close all", and "Expand selected" instead of "Open selected")
to ensure consistent UX across the backend UI.
| destruct() { | ||
| this.expandAllControl.removeEventListener('click', this.events.expandAll); | ||
| this.collapseAllControl.removeEventListener('click', this.events.collapseAll); | ||
| this.expandCheckedControl.removeEventListener('click', this.events.expandChecked); | ||
|
|
||
| this.toggles.forEach((toggle) => { | ||
| toggle.removeEventListener('click', this.events.toggle) | ||
| }); | ||
| } |
There was a problem hiding this comment.
destruct() will throw if tree action controls don't exist.
attachEvents() (lines 55–63) null-checks expandAllControl, collapseAllControl, and expandCheckedControl before adding listeners, but destruct() unconditionally calls removeEventListener on them. When quickTreeActions is false, these are null and destruct() will throw a TypeError.
Proposed fix
destruct() {
- this.expandAllControl.removeEventListener('click', this.events.expandAll);
- this.collapseAllControl.removeEventListener('click', this.events.collapseAll);
- this.expandCheckedControl.removeEventListener('click', this.events.expandChecked);
+ if (this.expandAllControl) {
+ this.expandAllControl.removeEventListener('click', this.events.expandAll);
+ }
+ if (this.collapseAllControl) {
+ this.collapseAllControl.removeEventListener('click', this.events.collapseAll);
+ }
+ if (this.expandCheckedControl) {
+ this.expandCheckedControl.removeEventListener('click', this.events.expandChecked);
+ }
this.toggles.forEach((toggle) => {
toggle.removeEventListener('click', this.events.toggle)
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/backend/formwidgets/relation/assets/js/src/Relation.js` around lines
73 - 81, destruct() currently assumes the tree action controls exist and calls
removeEventListener unconditionally, causing a TypeError when
expandAllControl/collapseAllControl/expandCheckedControl are null; update
destruct() to mirror attachEvents() by null-checking each of expandAllControl,
collapseAllControl, and expandCheckedControl before calling removeEventListener,
and keep the existing loop for this.toggles that removes events.toggle;
reference the destruct() and attachEvents() methods and the properties
expandAllControl, collapseAllControl, expandCheckedControl, toggles, and
events.toggle when making the change.
| updateScollBar(el) { | ||
| if (el === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| let openedLevel = el.classList.contains("checkboxlist-children") ? el : el.querySelector('.checkboxlist-children'); | ||
|
|
||
| openedLevel.addEventListener("transitionend", () => { | ||
| $('[data-control=scrollbar]').data('oc.scrollbar').update(); | ||
| }, {once: true}); | ||
| } |
There was a problem hiding this comment.
Potential TypeError if openedLevel is null, plus typo in method name.
Line 211: if el doesn't have class checkboxlist-children and has no such descendant, openedLevel will be null, and line 213 will throw. Also, updateScollBar should be updateScrollBar.
Proposed fix
- updateScollBar(el) {
+ updateScrollBar(el) {
if (el === undefined) {
return;
}
let openedLevel = el.classList.contains("checkboxlist-children") ? el : el.querySelector('.checkboxlist-children');
+ if (!openedLevel) {
+ return;
+ }
+
openedLevel.addEventListener("transitionend", () => {
$('[data-control=scrollbar]').data('oc.scrollbar').update();
}, {once: true});
}Also update all call sites (updateScollBar → updateScrollBar) on lines 128, 149, 172, and 197.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| updateScollBar(el) { | |
| if (el === undefined) { | |
| return; | |
| } | |
| let openedLevel = el.classList.contains("checkboxlist-children") ? el : el.querySelector('.checkboxlist-children'); | |
| openedLevel.addEventListener("transitionend", () => { | |
| $('[data-control=scrollbar]').data('oc.scrollbar').update(); | |
| }, {once: true}); | |
| } | |
| updateScrollBar(el) { | |
| if (el === undefined) { | |
| return; | |
| } | |
| let openedLevel = el.classList.contains("checkboxlist-children") ? el : el.querySelector('.checkboxlist-children'); | |
| if (!openedLevel) { | |
| return; | |
| } | |
| openedLevel.addEventListener("transitionend", () => { | |
| $('[data-control=scrollbar]').data('oc.scrollbar').update(); | |
| }, {once: true}); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/backend/formwidgets/relation/assets/js/src/Relation.js` around lines
206 - 216, The method updateScollBar has a typo and can throw a TypeError when
openedLevel is null; rename the method to updateScrollBar and update every call
site that uses updateScollBar, then inside the (now) updateScrollBar method
compute openedLevel as before but guard it with a null check (if openedLevel is
null/undefined return) before calling openedLevel.addEventListener; keep the
existing transitionend handler and options unchanged. Ensure all references in
this file that called updateScollBar (the earlier calls around the
checkbox/children handling) are renamed to updateScrollBar so usages and exports
stay consistent.
|
|
||
| // Top widget controls | ||
| .field-checkboxlist .checkboxlist-controls > div:nth-child(even) { | ||
| margin-left: auto; | ||
| margin-right: 0; | ||
| } |
There was a problem hiding this comment.
Fix empty line before comment (Stylelint).
Stylelint reports scss/double-slash-comment-empty-line-before on line 5. Remove the blank line 4 between the selector opening brace and the comment.
Proposed fix
div[data-control="relation"] {
-
// Top widget controls
.field-checkboxlist .checkboxlist-controls > div:nth-child(even) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Top widget controls | |
| .field-checkboxlist .checkboxlist-controls > div:nth-child(even) { | |
| margin-left: auto; | |
| margin-right: 0; | |
| } | |
| div[data-control="relation"] { | |
| // Top widget controls | |
| .field-checkboxlist .checkboxlist-controls > div:nth-child(even) { | |
| margin-left: auto; | |
| margin-right: 0; | |
| } | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 5-5: Unexpected empty line before comment (scss/double-slash-comment-empty-line-before)
(scss/double-slash-comment-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/backend/formwidgets/relation/assets/less/relation.less` around lines
4 - 9, Remove the empty line between the rule opening brace and the comment in
relation.less: locate the selector ".field-checkboxlist .checkboxlist-controls >
div:nth-child(even)" and the comment "// Top widget controls" and place the
comment immediately after the opening brace (no blank line) so the
scss/double-slash-comment-empty-line-before lint rule is satisfied.
| @@ -1,8 +1,11 @@ | |||
| <?php | |||
| $nameFrom = $this->nameFrom; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all places that render _field_checkboxlist.php
echo "=== Renderers of _field_checkboxlist.php ==="
rg -n '_field_checkboxlist' --type=php -C3Repository: wintercms/winter
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Search for nameFrom property definitions in widget classes
echo "=== Files defining nameFrom property ==="
rg -n 'nameFrom' --type=php -C2 | head -100Repository: wintercms/winter
Length of output: 7575
🏁 Script executed:
#!/bin/bash
# Check Form widget class structure
echo "=== Searching for Form widget class ==="
fd -e php -path '*/form/*' | grep -i widgetRepository: wintercms/winter
Length of output: 285
🏁 Script executed:
#!/bin/bash
# Search for makePartial and other render calls mentioning field_checkboxlist
rg 'field_checkboxlist' --type=phpRepository: wintercms/winter
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for Form widget class and its properties
rg -n 'class.*Form.*Widget|class Form' --type=php | head -20Repository: wintercms/winter
Length of output: 1864
🏁 Script executed:
#!/bin/bash
# Check if Form widget class defines nameFrom
rg -n 'class.*Form' --type=php -A 100 | rg 'nameFrom|public \$'Repository: wintercms/winter
Length of output: 10268
🏁 Script executed:
#!/bin/bash
# Search for checkboxlist field type usage and partials
rg -n "checkboxlist" --type=php | head -50Repository: wintercms/winter
Length of output: 2356
🏁 Script executed:
#!/bin/bash
# Search for makePartial calls in widget files
rg -n "makePartial.*field" --type=phpRepository: wintercms/winter
Length of output: 1620
🏁 Script executed:
#!/bin/bash
# Look at the Form widget's renderFormField method to see how it renders partials
rg -n "renderFormField|makePartial" modules/backend/widgets/Form.php -A 5Repository: wintercms/winter
Length of output: 1826
🏁 Script executed:
#!/bin/bash
# Check the Relation widget's _relation.php partial to see how it renders the field
cat -n modules/backend/formwidgets/relation/partials/_relation.phpRepository: wintercms/winter
Length of output: 331
🏁 Script executed:
#!/bin/bash
# Check if Form widget's renderField method also renders _field_checkboxlist.php
cat -n modules/backend/widgets/Form.php | sed -n '230,280p'Repository: wintercms/winter
Length of output: 1884
🏁 Script executed:
#!/bin/bash
# Check which widgets extend from Form widget or use checkboxlist fields with nameFrom
rg -n "type.*checkboxlist|field.*type.*checkboxlist" --type=php -B 5 -A 5Repository: wintercms/winter
Length of output: 2123
🏁 Script executed:
#!/bin/bash
# Look at the Form test to see how checkboxlist is used without nameFrom
cat -n modules/backend/tests/widgets/FormTest.php | sed -n '130,160p'Repository: wintercms/winter
Length of output: 1347
🏁 Script executed:
#!/bin/bash
# Check if the partial accesses nameFrom in a way that would fail without it
cat -n modules/backend/widgets/form/partials/_field_checkboxlist.php | sed -n '1,30p'Repository: wintercms/winter
Length of output: 1359
🏁 Script executed:
#!/bin/bash
# Check if there's a default nameFrom in the partial or if it's always expected
rg -n "nameFrom\|options\(\)" modules/backend/widgets/form/partials/_field_checkboxlist.php | head -10Repository: wintercms/winter
Length of output: 42
🏁 Script executed:
#!/bin/bash
# See the complete usage of nameFrom variable in the partial
cat -n modules/backend/widgets/form/partials/_field_checkboxlist.php | sed -n '1,10p'Repository: wintercms/winter
Length of output: 589
🏁 Script executed:
#!/bin/bash
# Check what happens when Form widget renders - does it pass any nameFrom context?
rg -n "renderFieldElement|field_checkboxlist" modules/backend/widgets/Form.php -B 5 -A 10Repository: wintercms/winter
Length of output: 564
🏁 Script executed:
#!/bin/bash
# Verify if makePartial in Form widget context has access to nameFrom or passes it
rg -n "makePartial.*field.*checkboxlist|renderFieldElement" modules/backend/widgets/Form.php -A 15 | head -40Repository: wintercms/winter
Length of output: 458
🏁 Script executed:
#!/bin/bash
# Check how $nameFrom is used throughout the partial - especially the dangerous line
rg -n "\$nameFrom" modules/backend/widgets/form/partials/_field_checkboxlist.phpRepository: wintercms/winter
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if Form widget class actually inherits nameFrom from a parent class
rg -n "class Form|nameFrom" modules/backend/widgets/Form.php | head -20Repository: wintercms/winter
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check what the parent class WidgetBase provides
fd -e php -name "WidgetBase.php" | xargs rg -n "public \$nameFrom|private \$nameFrom"Repository: wintercms/winter
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Find WidgetBase class
fd -e php -name "WidgetBase.php"Repository: wintercms/winter
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Check the parent WidgetBase class for nameFrom property
find . -name "WidgetBase.php" -type f | xargs cat | head -100Repository: wintercms/winter
Length of output: 2488
🏁 Script executed:
#!/bin/bash
# Check complete usage of nameFrom in the checkboxlist partial
grep -n "nameFrom" modules/backend/widgets/form/partials/_field_checkboxlist.phpRepository: wintercms/winter
Length of output: 466
🏁 Script executed:
#!/bin/bash
# Check if there are any default values or fallbacks for nameFrom
rg -n "nameFrom" modules/backend/widgets/Form.phpRepository: wintercms/winter
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check what happens when Form widget renders checkboxlist - verify if it actually uses nameFrom
cat -n modules/backend/widgets/form/partials/_field_checkboxlist.php | sed -n '1,50p'Repository: wintercms/winter
Length of output: 2247
🏁 Script executed:
#!/bin/bash
# Look at the complete section where nameFrom is used to understand when it matters
cat -n modules/backend/widgets/form/partials/_field_checkboxlist.php | sed -n '100,150p'Repository: wintercms/winter
Length of output: 2951
🏁 Script executed:
#!/bin/bash
# Check if there's any error suppression or try/catch when accessing undefined properties
rg -n "error_reporting|@\$this" modules/backend/widgets/form/partials/_field_checkboxlist.phpRepository: wintercms/winter
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Verify by running the Form widget test to see if it actually passes
git log --oneline modules/backend/tests/widgets/FormTest.php | head -5Repository: wintercms/winter
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Let's verify: Does Form widget actually get used with complex checkboxlist options with children?
rg -n "displayTree|children" modules/backend/tests/widgets/FormTest.phpRepository: wintercms/winter
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there are integration tests that actually render Form with checkboxlist having nameFrom-like options
rg -n "nameFrom|checkboxlist" modules/backend/tests/ -lRepository: wintercms/winter
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Final check: What happens when PHP accesses undefined property?
# Let's see if there's any property_exists check or default handling
cat -n modules/backend/classes/FormWidgetBase.php | head -50Repository: wintercms/winter
Length of output: 1460
🏁 Script executed:
#!/bin/bash
# Final verification: does the Form widget test actually pass and render the checkbox content?
cat -n modules/backend/tests/widgets/FormTest.php | sed -n '131,160p'Repository: wintercms/winter
Length of output: 1339
🏁 Script executed:
#!/bin/bash
# Check what error_reporting or error handling exists in Winter CMS
rg -n "error_reporting|E_ALL" modules/backend/ -l | head -5Repository: wintercms/winter
Length of output: 311
🏁 Script executed:
#!/bin/bash
# Verify if undefined property access is being suppressed or if there's a __get magic method
rg -n "__get|__isset|error" modules/backend/classes/FormWidgetBase.phpRepository: wintercms/winter
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Final check: Verify if Extendable base class or traits provide __get magic method
rg -n "class Extendable|__get|__isset" --type=php | grep -E "Extendable|__get|__isset" | head -20Repository: wintercms/winter
Length of output: 1041
🏁 Script executed:
#!/bin/bash
# Check if the test actually triggers the nameFrom code path
cat -n modules/backend/widgets/form/partials/_field_checkboxlist.php | sed -n '80,100p'Repository: wintercms/winter
Length of output: 862
🏁 Script executed:
#!/bin/bash
# Verify the condition - does renderCheckboxLine get called from Form widget test?
# Check if the test uses displayTree or structured options
cat -n modules/backend/tests/widgets/FormTest.php | sed -n '131,160p'Repository: wintercms/winter
Length of output: 1339
🏁 Script executed:
#!/bin/bash
# Final verification: does accessing undefined property throw error or just warning?
# Check PHP error handling in the codebase
rg -n "error_reporting\|ini_set.*error" modules/backend/ --type=php | head -3Repository: wintercms/winter
Length of output: 42
🏁 Script executed:
#!/bin/bash
# See the exact code path: when does nameFrom actually get used?
cat -n modules/backend/widgets/form/partials/_field_checkboxlist.php | sed -n '90,150p'Repository: wintercms/winter
Length of output: 3285
🏁 Script executed:
#!/bin/bash
# Final check: confirm PHP will generate an error when accessing undefined property
# Check if displayTree defaults to false and when simple options don't trigger the nameFrom usage
cat -n modules/backend/widgets/form/partials/_field_checkboxlist.php | sed -n '1,15p'Repository: wintercms/winter
Length of output: 741
🏁 Script executed:
#!/bin/bash
# Verify if the test actually exercises the nameFrom code path
# Look at what field->options() returns
rg -n "options\(\)|displayTree" modules/backend/classes/FormField.php | head -10Repository: wintercms/winter
Length of output: 42
Fix undefined property access: $this->nameFrom doesn't exist on Form widget.
The partial unconditionally accesses $this->nameFrom on line 2, but this property only exists on Relation, TagList, and RecordFinder widgets—not on Form widget. When Form renders a checkboxlist field, this will generate a PHP notice. The property is actually used in lines 126+ (when displayTree is true) and should either be conditionally accessed with a fallback or passed as a variable when the partial is rendered from different widget contexts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/backend/widgets/form/partials/_field_checkboxlist.php` at line 2, The
partial currently unconditionally reads $this->nameFrom which doesn't exist on
Form widget; change the assignment so it only reads $this->nameFrom when
available and otherwise uses a passed-in fallback (e.g. a local $nameFrom
variable) or null — do not access $this->nameFrom on Form contexts. Ensure the
conditional access is applied before any use (notably where displayTree is
handled around the block that references nameFrom later), and update the partial
to accept/use a provided $nameFrom when rendered from widgets that don't expose
$this->nameFrom.
| function renderCheckboxLine( | ||
| $field, | ||
| array $checkedValues, | ||
| array $fieldOptions, | ||
| string $nameFrom, | ||
| bool $readOnly, | ||
| int &$index | ||
| ) { | ||
| foreach ($fieldOptions as $value => $option): | ||
| $index++; | ||
| $checkboxId = 'checkbox_'. $field->getId() .'_'. $index; | ||
|
|
||
| if (!is_array($option)) { | ||
| $option = [$option]; | ||
| } | ||
| $children = array_get($option, 'children', []); | ||
| ?> | ||
|
|
||
| <div class="checkboxlist-item"> | ||
|
|
||
| <div class="checkbox custom-checkbox"> | ||
| <input | ||
| type="checkbox" | ||
| id="<?= $checkboxId ?>" | ||
| name="<?= $field->getName() ?>[]" | ||
| value="<?= e($value) ?>" | ||
| <?= $readOnly ? 'disabled="disabled"' : '' ?> | ||
| <?= in_array($value, $checkedValues) ? 'checked="checked"' : '' ?>> | ||
|
|
||
| <label for="<?= $checkboxId ?>"> | ||
| <?= e(trans($option[$nameFrom])) ?> | ||
| </label> | ||
| </div> | ||
|
|
||
| <?php if (count($children) > 0): ?> | ||
| <a href="javascript:;" class="checkboxlist-item-toggle"> | ||
| <i class="icon-chevron-right"></i> | ||
| </a> | ||
| <div class="checkboxlist-children"> | ||
| <div id="<?= $checkboxId ?>_children"> | ||
| <?php e(renderCheckboxLine($field, $checkedValues, $children, $nameFrom, $readOnly, $index)); ?> | ||
| </div> | ||
| </div> | ||
| <?php endif ?> | ||
| </div> | ||
| <?php | ||
| endforeach; | ||
| } |
There was a problem hiding this comment.
Fatal error if two displayTree relations are on the same page — renderCheckboxLine is a named function.
Defining a named function inside a template means PHP will throw Cannot redeclare renderCheckboxLine() if this partial is rendered more than once in the same request (e.g., two relation fields with displayTree enabled on the same form). Convert this to a closure assigned to a variable.
Proposed fix — use a closure instead
- function renderCheckboxLine(
+ $renderCheckboxLine = function (
$field,
array $checkedValues,
array $fieldOptions,
string $nameFrom,
bool $readOnly,
int &$index
- ) {
+ ) use (&$renderCheckboxLine) {
foreach ($fieldOptions as $value => $option):
$index++;
$checkboxId = 'checkbox_'. $field->getId() .'_'. $index;
if (!is_array($option)) {
$option = [$option];
}
$children = array_get($option, 'children', []);
?>
<div class="checkboxlist-item">
<div class="checkbox custom-checkbox">
<input
type="checkbox"
id="<?= $checkboxId ?>"
name="<?= $field->getName() ?>[]"
value="<?= e($value) ?>"
<?= $readOnly ? 'disabled="disabled"' : '' ?>
<?= in_array($value, $checkedValues) ? 'checked="checked"' : '' ?>>
<label for="<?= $checkboxId ?>">
<?= e(trans($option[$nameFrom])) ?>
</label>
</div>
<?php if (count($children) > 0): ?>
<a href="javascript:;" class="checkboxlist-item-toggle">
<i class="icon-chevron-right"></i>
</a>
<div class="checkboxlist-children">
<div id="<?= $checkboxId ?>_children">
- <?php e(renderCheckboxLine($field, $checkedValues, $children, $nameFrom, $readOnly, $index)); ?>
+ <?php $renderCheckboxLine($field, $checkedValues, $children, $nameFrom, $readOnly, $index); ?>
</div>
</div>
<?php endif ?>
</div>
<?php
endforeach;
- }
+ };
- renderCheckboxLine($field, $checkedValues, $fieldOptions, $nameFrom, $readOnly, $index);
+ $renderCheckboxLine($field, $checkedValues, $fieldOptions, $nameFrom, $readOnly, $index);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/backend/widgets/form/partials/_field_checkboxlist.php` around lines
96 - 143, The named function renderCheckboxLine causes a fatal redeclaration
when the partial is included multiple times; replace it with a recursive
closure: define $renderCheckboxLine = function($field, array $checkedValues,
array $fieldOptions, string $nameFrom, bool $readOnly, int &$index) use
(&$renderCheckboxLine) { ... } containing the same body, and update the
recursive call and any invocations to call $renderCheckboxLine(...) (remove the
wrapping e(...) used for renderCheckboxLine since the closure echoes directly)
so the template uses the closure instead of a global function.
| </a> | ||
| <div class="checkboxlist-children"> | ||
| <div id="<?= $checkboxId ?>_children"> | ||
| <?php e(renderCheckboxLine($field, $checkedValues, $children, $nameFrom, $readOnly, $index)); ?> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unnecessary e() wrapper around renderCheckboxLine call.
renderCheckboxLine outputs HTML directly as a side effect and returns void. Wrapping it in e() is misleading—e(null) just produces an empty string. Remove the e() call.
Proposed fix
- <?php e(renderCheckboxLine($field, $checkedValues, $children, $nameFrom, $readOnly, $index)); ?>
+ <?php renderCheckboxLine($field, $checkedValues, $children, $nameFrom, $readOnly, $index); ?>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/backend/widgets/form/partials/_field_checkboxlist.php` at line 136,
The call to e(renderCheckboxLine(...)) is incorrect because renderCheckboxLine()
echoes HTML and returns void; remove the unnecessary e() wrapper and call
renderCheckboxLine($field, $checkedValues, $children, $nameFrom, $readOnly,
$index) directly in the template so the output is rendered as intended, and if
any escaping is actually required wrap the producing function to return a string
instead of echoing before escaping.
3 participants
This allow relation model with nested tree trait to display a expandable/collapsable list of checkboxes.
The field config allow two new options :
Summary by CodeRabbit