Added expand/collapse functionalities to relationship formwidget#1374
Open
damsfx wants to merge 14 commits intowintercms:developfrom
Open
Added expand/collapse functionalities to relationship formwidget#1374damsfx wants to merge 14 commits intowintercms:developfrom
damsfx wants to merge 14 commits intowintercms:developfrom
Conversation
Member
|
@damsfx ooo, I like this. And in Snowboard too! 😍 Couple of quick comments/questions:
|
Contributor
Author
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. |
Contributor
Author
|
Latest update :
|
Member
|
@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. |
Member
|
@bennothommo are you happy with this as it is? @damsfx is there a related docs PR? |
LukeTowers
reviewed
Jul 16, 2025
LukeTowers
added
needs review
Contributor
Author
Not yet, but I can add a pending PR for this. |
Co-authored-by: Luke Towers <github@luketowers.ca>
Contributor
Author
@LukeTowers Done : wintercms/docs#243 |
damsfx
added a commit
to Hounddd/wn-enhancedrelation-plugin
that referenced
this pull request
Aug 8, 2025
Contributor
Author
|
@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. |
Member
|
@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
approved these changes
Sep 9, 2025
Member
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
Contributor
Author
😱🙄 Oh, I was really distracted! Done ! |
Contributor
Author
|
@LukeTowers @bennothommo Is this still an interesting PR for WinterCMS !? |
Member
|
@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
WalkthroughAdds a tree-capable Relation form widget: backend config and asset loading, new frontend Relation JS plugin for expand/collapse and selection actions, template updates for tree rendering, styles for nested checkbox lists, and translation keys for tree controls. Changes
Sequence Diagram(s)mermaid 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.
-Treeview nw compatible with a custom SQL select statement to use for the name
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
modules/backend/formwidgets/Relation.php (2)
52-55:$displayTreeshould be explicitly initialised tofalse.The property is left at
null, relying on PHP's falsy behaviour. The doc-comment says "Define if the widget must be rendered has a displayTree" (boolean), sofalsebetter communicates intent and is consistent with the documented type.🛠️ Proposed fix
- public $displayTree; + public $displayTree = false;Also, the doc comment has a grammar slip: "rendered has a displayTree" → "rendered as a tree".
🤖 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, The public property $displayTree in class Relation is left uninitialized (null) but documented as a boolean; explicitly initialize it to false (public $displayTree = false;) to match the doc type and intent, and update the doc-comment text from "rendered has a displayTree" to "rendered as a tree" to fix the grammar and clarify meaning.
163-164:class_uses()misses tree traits inherited from parent classes — useclass_uses_recursive().
class_uses($relationModel)only returns traits used directly on that class. If a model extends a base class that usesNestedTreeorSimpleTree, the detection silently returnsfalseand the tree path is never taken. Laravel'sclass_uses_recursive()traverses the entire inheritance chain.🛠️ 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 trait check for tree models currently uses class_uses($relationModel) which only inspects traits on the class itself; replace it with class_uses_recursive($relationModel) so inherited traits on parent classes are detected. Update the $usesTree computation (the line using $treeTraits and $usesTree) to call class_uses_recursive($relationModel) instead of class_uses($relationModel) so NestedTree and SimpleTree traits inherited from base models are recognized.modules/backend/formwidgets/relation/assets/js/src/Relation.js (2)
190-190: Typo:tooglePromise→togglePromise.🛠️ Proposed fix
- const tooglePromise = new Promise((resolve, reject) => { + const togglePromise = new Promise((resolve, reject) => {- tooglePromise.then((parent) => { + togglePromise.then((parent) => {🤖 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` at line 190, Rename the misspelled variable tooglePromise to togglePromise in Relation.js: update the Promise declaration (const tooglePromise) and all usages/references to that variable (including any places that await, resolve, reject, or return it) so the identifier is consistent throughout the Relation component (e.g., where tooglePromise is created and later referenced).
104-104: Doc typo: "signle" → "single".- * Close an signle level of the tree + * Close a single level of the tree🤖 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` at line 104, Fix the documentation typo in Relation.js: change the comment "Close an signle level of the tree" to "Close a single level of the tree" (the comment immediately above the function that closes a tree level, e.g., the closeLevel/closeOneLevel method in Relation.js). Ensure only the wording is corrected and no code logic is changed.modules/backend/formwidgets/relation/assets/less/relation.less (1)
13-13: Extraneous space before colon in custom property declaration.
--background-padding : 10pxshould not have a space before the colon.🛠️ Proposed fix
- --background-padding : 10px; + --background-padding: 10px;🤖 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` at line 13, The custom CSS property declaration "--background-padding : 10px" has an extraneous space before the colon; update the declaration of the --background-padding variable in relation.less (the line defining --background-padding) to remove the space before the colon so it follows the correct "--background-padding: 10px" formatting.modules/backend/formwidgets/relation/assets/less/relation.css (1)
1-55:relation.cssin theless/directory duplicatesrelation.lessoutput — risk of divergence.
Relation.jsalready importsrelation.lessdirectly for the build pipeline, andloadAssets()only enqueues JS. Therelation.cssfile appears to be a manually maintained flat-CSS copy of the compiled LESS rules. Ifrelation.lessis updated without updatingrelation.css, they will silently diverge. Clarify the intended role of this file, or remove it in favour of the LESS source alone.🤖 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.css` around lines 1 - 55, The relation.css file is a duplicated compiled output of relation.less and risks divergence; update the codebase so there's a single source-of-truth: either delete relation.css and ensure Relation.js (or the asset build pipeline invoked by loadAssets()) imports/serves the compiled relation.less output at build time, or mark relation.css as a generated artifact and add a build step that compiles relation.less into relation.css automatically. Modify Relation.js/loadAssets() or the build config to reference the generated CSS (not a manually maintained relation.css) and remove the manual file if you choose the first option.
🤖 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/assets/js/src/Relation.js`:
- Around line 212-226: The updateScrollBar method uses a global selector and
calls .data('oc.scrollbar').update() unsafely; change it to scope the scrollbar
lookup to this.element (e.g. use this.element.find('[data-control=scrollbar]'))
and, after selecting the jQuery element, retrieve the plugin instance and guard
before calling update (check that the jQuery selection is non-empty and that
selection.data('oc.scrollbar') is defined) inside the transitionend handler in
updateScrollBar so expanding/collapsing only updates the local scrollbar and
avoids TypeError when no plugin exists.
In `@modules/backend/formwidgets/relation/assets/less/relation.css`:
- Line 7: Stylelint flags are due to missing blank lines after CSS custom
property declarations before the next regular declarations; locate the custom
property declarations in relation.css (the --* lines) that precede declarations
like the "display: flex;" rule and insert a single empty line between each
custom-property block and the following regular property(s) so the
declaration-empty-line-before rule is satisfied.
In `@modules/backend/widgets/form/partials/_field_checkboxlist.php`:
- Line 146: Remove the leftover commented debug statements in the checkbox list
partial: delete the lines containing "// debug($fieldOptions)" and "//
debug($option)" in the _field_checkboxlist.php partial (and any other similar
"// debug(...)" calls in that file) so the production code contains no
commented-out debug artifacts.
- Line 93: The tree rendering path initializes $index = 1 which causes tree
checkbox IDs to start at 2; change the tree-mode initialization of the $index
variable from 1 to 0 so it matches flat mode and yields the first checkbox ID as
checkbox_..._1 (update the $index assignment in the tree branch inside
_field_checkboxlist.php).
---
Nitpick comments:
In `@modules/backend/formwidgets/Relation.php`:
- Around line 52-55: The public property $displayTree in class Relation is left
uninitialized (null) but documented as a boolean; explicitly initialize it to
false (public $displayTree = false;) to match the doc type and intent, and
update the doc-comment text from "rendered has a displayTree" to "rendered as a
tree" to fix the grammar and clarify meaning.
- Around line 163-164: The trait check for tree models currently uses
class_uses($relationModel) which only inspects traits on the class itself;
replace it with class_uses_recursive($relationModel) so inherited traits on
parent classes are detected. Update the $usesTree computation (the line using
$treeTraits and $usesTree) to call class_uses_recursive($relationModel) instead
of class_uses($relationModel) so NestedTree and SimpleTree traits inherited from
base models are recognized.
In `@modules/backend/formwidgets/relation/assets/js/src/Relation.js`:
- Line 190: Rename the misspelled variable tooglePromise to togglePromise in
Relation.js: update the Promise declaration (const tooglePromise) and all
usages/references to that variable (including any places that await, resolve,
reject, or return it) so the identifier is consistent throughout the Relation
component (e.g., where tooglePromise is created and later referenced).
- Line 104: Fix the documentation typo in Relation.js: change the comment "Close
an signle level of the tree" to "Close a single level of the tree" (the comment
immediately above the function that closes a tree level, e.g., the
closeLevel/closeOneLevel method in Relation.js). Ensure only the wording is
corrected and no code logic is changed.
In `@modules/backend/formwidgets/relation/assets/less/relation.css`:
- Around line 1-55: The relation.css file is a duplicated compiled output of
relation.less and risks divergence; update the codebase so there's a single
source-of-truth: either delete relation.css and ensure Relation.js (or the asset
build pipeline invoked by loadAssets()) imports/serves the compiled
relation.less output at build time, or mark relation.css as a generated artifact
and add a build step that compiles relation.less into relation.css
automatically. Modify Relation.js/loadAssets() or the build config to reference
the generated CSS (not a manually maintained relation.css) and remove the manual
file if you choose the first option.
In `@modules/backend/formwidgets/relation/assets/less/relation.less`:
- Line 13: The custom CSS property declaration "--background-padding : 10px" has
an extraneous space before the colon; update the declaration of the
--background-padding variable in relation.less (the line defining
--background-padding) to remove the space before the colon so it follows the
correct "--background-padding: 10px" formatting.
| } | ||
| div[data-control="relation"] .checkboxlist-item { | ||
| --background-padding: 10px; | ||
| display: flex; |
There was a problem hiding this comment.
Stylelint declaration-empty-line-before errors (lines 7 and 50).
After a CSS custom property declaration, Stylelint requires an empty line before the following regular declaration.
🛠️ Proposed fix
div[data-control="relation"] .checkboxlist-item {
--background-padding: 10px;
+
display: flex; div[data-control="relation"] .checkboxlist-item .checkboxlist-item {
--background-padding: 20px;
+
margin-left: 10px;Also applies to: 50-50
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 7-7: Expected empty line before declaration (declaration-empty-line-before)
(declaration-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.css` at line 7,
Stylelint flags are due to missing blank lines after CSS custom property
declarations before the next regular declarations; locate the custom property
declarations in relation.css (the --* lines) that precede declarations like the
"display: flex;" rule and insert a single empty line between each
custom-property block and the following regular property(s) so the
declaration-empty-line-before rule is satisfied.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/backend/widgets/form/partials/_field_checkboxlist.php (1)
11-43:⚠️ Potential issue | 🟠 MajorRead-only path silently drops all checked nested (children) items.
The
foreachat line 16 iterates only the top-level keys of$fieldOptions. WhendisplayTreeis enabled, the options contain a'children'key with nested descendants. Any item that is checked at depth > 1 passes thein_array($value, $checkedValues)guard (correctly), but the loop never visits it — so it is never rendered. A user will see an empty or partial read-only list even though nested values are persisted.🐛 Proposed fix — recurse through tree children in the read-only path
<?php if ($readOnly && $field->value): ?> <div class="field-checkboxlist"> <?php $index = 0; - foreach ($fieldOptions as $value => $option): - $index++; - $checkboxId = 'checkbox_'.$field->getId().'_'.$index; - if (!in_array($value, $checkedValues)) { - continue; - } - if (!is_array($option)) { - $option = [$option]; - } - ?> - <div class="checkbox custom-checkbox"> - <input - type="checkbox" - id="<?= $checkboxId ?>" - name="<?= $field->getName() ?>[]" - value="<?= e($value) ?>" - disabled="disabled" - checked="checked"> - - <label for="<?= $checkboxId ?>"> - <?= e(trans($option[0])) ?> - </label> - <?php if (isset($option[1])): ?> - <p class="help-block"><?= e(trans($option[1])) ?></p> - <?php endif ?> - </div> - <?php endforeach ?> + $renderReadOnlyLine = function (array $options) use (&$renderReadOnlyLine, &$index, $field, $checkedValues): void { + foreach ($options as $value => $option): + $index++; + $checkboxId = 'checkbox_'.$field->getId().'_'.$index; + if (!is_array($option)) { + $option = [$option]; + } + $children = array_get($option, 'children', []); + if (in_array($value, $checkedValues)): + ?> + <div class="checkbox custom-checkbox"> + <input + type="checkbox" + id="<?= $checkboxId ?>" + name="<?= $field->getName() ?>[]" + value="<?= e($value) ?>" + disabled="disabled" + checked="checked"> + <label for="<?= $checkboxId ?>"> + <?= e(trans($option[0])) ?> + </label> + <?php if (isset($option[1])): ?> + <p class="help-block"><?= e(trans($option[1])) ?></p> + <?php endif ?> + </div> + <?php + endif; + if (!empty($children)) { + $renderReadOnlyLine($children); + } + endforeach; + }; + $renderReadOnlyLine($fieldOptions); ?> </div>🤖 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 11 - 43, The read-only loop over $fieldOptions only iterates top-level items and thus skips nested 'children' entries; fix by replacing the flat foreach with a recursive renderer (e.g. a local function/closure like renderCheckedOptions) that accepts $options and a reference to $index, walks each node, checks in_array($value, $checkedValues) and renders the checkbox/label/help-block when matched, and if an item has a 'children' key calls itself on that children array; ensure $checkboxId uses the shared $index (incremented in the recursive function) so IDs remain unique.
🧹 Nitpick comments (2)
modules/backend/widgets/form/partials/_field_checkboxlist.php (2)
124-124: Inconsistency:reset($option)for label vs$option[0]used everywhere else.The tree path uses
reset($option)to retrieve the label, while the flat editable path (line 162) and the flat read-only path (line 36) both use$option[0]. For the expected array shape['Label', 'children' => [...]]they produce the same value, butreset()mutates the internal array pointer and signals a different access pattern to readers. Use$option[0]for consistency.♻️ Proposed fix
- <?= e(trans(reset($option))) ?> + <?= e(trans($option[0])) ?>🤖 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 124, The template uses reset($option) to render the label in the tree path, which mutates the array pointer and is inconsistent with other places that use $option[0]; replace the reset($option) usage in _field_checkboxlist.php with $option[0] so the label is accessed non-destructively and matches the flat editable/read-only paths that reference $option[0].
112-138: Tree nodes silently discard$option[1]help text.The flat rendering path (lines 164–166) renders a
.help-blockparagraph when$option[1]is set. The tree rendering closure has no equivalent — option arrays with a description at index 1 will silently lose that text for tree nodes.♻️ Proposed fix — add help text after the label in the tree path
<label for="<?= $checkboxId ?>"> <?= e(trans($option[0])) ?> </label> + <?php if (isset($option[1])): ?> + <p class="help-block"><?= e(trans($option[1])) ?></p> + <?php endif ?> </div>🤖 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 112 - 138, The tree rendering path for checkbox list items omits option help text stored in $option[1]; update the tree branch inside the checkboxlist-item (the block that renders the <input> and <label> where $checkboxId, $option and $children are used) to output the same help paragraph as the flat path — render a <p class="help-block"> with the escaped/transformed $option[1] immediately after the <label> when isset($option[1])/!empty($option[1]); ensure this change mirrors the flat rendering behavior and does not affect the existing $renderCheckboxLine recursive call for children.
🤖 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/widgets/form/partials/_field_checkboxlist.php`:
- Around line 64-72: The partial _field_checkboxlist.php renders tree controls
(data-field-checkboxlist-expand-all, data-field-checkboxlist-expand-checked,
data-field-checkboxlist-collapse-all and checkboxlist-item-toggle anchors)
whenever field.displayTree is truthy, but the click handlers live only in
modules/backend/formwidgets/relation/assets/js/src/Relation.js and are not
loaded for other widgets; fix by either validating/enforcing displayTree to be
used only with the Relation widget at form field creation (e.g., in
createFormField or the FormField factory: check field.displayTree and throw or
clear it unless the widget is Relation) or extract the tree logic from
Relation.js into a standalone asset (e.g., checkboxlist-tree.js) and load that
script from the partial when field.displayTree is true so the handlers for the
data-field-checkboxlist-* and checkboxlist-item-toggle attributes are always
registered.
---
Outside diff comments:
In `@modules/backend/widgets/form/partials/_field_checkboxlist.php`:
- Around line 11-43: The read-only loop over $fieldOptions only iterates
top-level items and thus skips nested 'children' entries; fix by replacing the
flat foreach with a recursive renderer (e.g. a local function/closure like
renderCheckedOptions) that accepts $options and a reference to $index, walks
each node, checks in_array($value, $checkedValues) and renders the
checkbox/label/help-block when matched, and if an item has a 'children' key
calls itself on that children array; ensure $checkboxId uses the shared $index
(incremented in the recursive function) so IDs remain unique.
---
Nitpick comments:
In `@modules/backend/widgets/form/partials/_field_checkboxlist.php`:
- Line 124: The template uses reset($option) to render the label in the tree
path, which mutates the array pointer and is inconsistent with other places that
use $option[0]; replace the reset($option) usage in _field_checkboxlist.php with
$option[0] so the label is accessed non-destructively and matches the flat
editable/read-only paths that reference $option[0].
- Around line 112-138: The tree rendering path for checkbox list items omits
option help text stored in $option[1]; update the tree branch inside the
checkboxlist-item (the block that renders the <input> and <label> where
$checkboxId, $option and $children are used) to output the same help paragraph
as the flat path — render a <p class="help-block"> with the escaped/transformed
$option[1] immediately after the <label> when
isset($option[1])/!empty($option[1]); ensure this change mirrors the flat
rendering behavior and does not affect the existing $renderCheckboxLine
recursive call for children.
Contributor
Author
|
@LukeTowers Done some CodeRabbit's suggestions after a merge from develop branch. About him, he's a bit temperamental because every commit causes a bit of hysteria. Minor change: Pending related PRs: |
LukeTowers
reviewed
Feb 20, 2026
Member
There was a problem hiding this comment.
What is compiling this file?
Contributor
Author
There was a problem hiding this comment.
Good catch!
Completely useless file, probably compiled by an extension when opening the less file.
Sorry for not paying attention to that.
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
3 participants
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.
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