Multiple fixes#440
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDie PR führt gemeinsame Helper für Label-, Layout- und Feldtyp-Logik ein, gleicht Parser und Flex-Repeater in den Renderingpfaden an, aktualisiert Modal-Wrapper und Selectpicker, stellt CSS auf Variablen um und ergänzt Demo, Doku, Migration und Changelog für Version 9.3.0. ChangesRenderer-Parität und Begleitänderungen
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR aligns the classic parser and the Flex-Repeater template renderer to reduce markup drift (rows/columns, modals, tooltips, collapse, tabs) and adds a dedicated parity demo page plus supporting docs/assets updates.
Changes:
- Introduces shared internal helpers for layout/attribute handling (
MFormLayoutCore) and label/tooltip rendering (MFormLabelRenderer) and applies them to parser + Flex-Repeater. - Aligns Flex-Repeater output with classic parser behavior for column auto-grouping, modal wrappers, collapse/tab flags, and
setTooltipInfo()defaults. - Adds a renderer-parity demo page + translations, updates docs/changelog, and stabilizes selectpicker container handling; large CSS refactor to CSS variables/dark-mode parity.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pages/demo.demo_renderer_parity.php | New backend demo page to compare Parser vs Flex-Repeater output markers. |
| package.yml | Version bump to 9.3.0 and registers the new demo subpage. |
| lib/MForm/Template/MFormLayoutCore.php | New shared core for row-class consumption and collapse flag logic. |
| lib/MForm/Template/MFormLabelRenderer.php | New shared label/tooltip renderer (default icon change to fa-info-circle). |
| lib/MForm/Parser/MFormParser.php | Refactors parser wrappers to use shared label/layout logic and aligns collapse/tab handling. |
| lib/MForm/Migration/MBlockToRepeaterConverter.php | Minor robustness change around preg_match_all result handling. |
| lib/MForm/FlexRepeater/MFormFlexRepeaterRenderer.php | Aligns Flex template generation with parser: column auto-grouping, modal row classes, collapse/tab truthy flags, label rendering, and setFull() handling. |
| lang/en_gb.lang | Adds i18n strings for the new parity demo page/sections. |
| lang/de_de.lang | Adds i18n strings for the new parity demo page/sections. |
| fragments/mform/mform_wrapper.php | Modal wrapper now renders as row form-group and supports optional row classes via attributes. |
| docs/13_api_reference.md | Documents the new default tooltip icon behavior. |
| docs/05_wrapper.md | Documents column auto-row behavior in Flex-Repeater + new modal/column row-class attributes. |
| docs/01_basics.md | Adds tooltip icon default/override documentation and examples. |
| CHANGELOG.md | Adds release notes for 9.3.0 covering the rendering parity fixes/features. |
| assets/mform.js | Ensures selectpicker defaults to data-container="body" when not explicitly set. |
| assets/css/mform.css | Refactors styling to CSS variables and improves dark-mode consistency across components. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
assets/css/mform.css (1)
5-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDer komplette Variablenblock zwischen
body.rex-theme-dark(68-128) und@media (prefers-color-scheme: dark)(130-192) ist 1:1 dupliziert. Das ist für reines CSS ohne Preprocessor kaum anders lösbar (Media-Query-Grenze verhindert Selektor-Merge), aber jede zukünftige Anpassung muss zwingend an beiden Stellen synchron gepflegt werden – ein Risiko für Drift. Falls ein Preprocessor (SCSS/LESS) im Build verfügbar ist, wäre eine gemeinsame Mixin/Variable hier sinnvoll.🤖 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 `@assets/css/mform.css` around lines 5 - 192, The dark-theme variable block is duplicated in both the body.rex-theme-dark rule and the `@media` (prefers-color-scheme: dark) override, so keep the two sections in sync by extracting the shared declarations into a single source of truth if the build supports it (for example via a preprocessor mixin or shared variable block) and then reference that in both selectors; use the duplicated CSS variable groups under .mform/.mfr-container as the target for the cleanup.lib/MForm/FlexRepeater/MFormFlexRepeaterRenderer.php (1)
595-616: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMoin.
resolveAnyLabel()ist eine 1:1-Kopie vonMFormLabelRenderer::resolveLabelValue()(und laut Graph existiert eine dritte Kopie inMFormParser). DaMFormLabelRendererhier ohnehin schon importiert ist, lässt sich die Logik zentralisieren und Divergenz vermeiden.♻️ Vorschlag: gemeinsamen Helper wiederverwenden
private static function getLabelString(mixed $label): string { - return self::resolveAnyLabel($label); - } - - private static function resolveAnyLabel(mixed $label): string - { - if (!is_array($label)) { - return (string) ($label ?? ''); - } - - foreach ($label as $key => $itemLabel) { - if (is_string($key) && str_contains(rex_i18n::getLocale(), $key)) { - return is_array($itemLabel) - ? (string) (array_values($itemLabel)[0] ?? '') - : (string) $itemLabel; - } - } - - $first = array_values($label)[0] ?? ''; - return is_array($first) ? (string) (array_values($first)[0] ?? '') : (string) $first; + return MFormLabelRenderer::resolveLabelValue($label); }🤖 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 `@lib/MForm/FlexRepeater/MFormFlexRepeaterRenderer.php` around lines 595 - 616, The label resolution logic in getLabelString()/resolveAnyLabel() is duplicated from MFormLabelRenderer::resolveLabelValue() and should be centralized. Replace the local implementation in MFormFlexRepeaterRenderer with the shared MFormLabelRenderer helper already available via the import, and keep MFormParser aligned by reusing the same label resolution path so the behavior stays consistent and avoids future divergence.lib/MForm/Parser/MFormParser.php (1)
277-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
MFormLayoutCore::consumeCollapseWrapperAttributesbleibt hier ungenutzt.Diese Methode wurde laut Kontext extra eingeführt, um
data-group-hide-toggle-links,data-group-accordionunddata-group-open-collapsekonsistent zu entfernen, und wird im Flex-Repeater-Pfad bereits genutzt (MFormFlexRepeaterRenderer.php:148). Der Parser pflegt in Zeile 279 stattdessen weiterhin eine eigene, literal duplizierte Keyliste. Für echte Parität sollte der Parser denselben Helper aufrufen statt die drei Keys ein zweites Mal hart zu codieren.♻️ Vorschlag
- if ('collapse' == $item->getType()) { - $removeAttributes = ['data-group-hide-toggle-links', 'data-group-accordion', 'data-group-open-collapse']; - $openCollapse = MFormLayoutCore::isCollapseOpen($attributes); + if ('collapse' == $item->getType()) { + $openCollapse = MFormLayoutCore::isCollapseOpen($attributes); $isAccordion = MFormLayoutCore::isCollapseAccordion($attributes); $hideToggle = MFormLayoutCore::shouldHideCollapseToggle($attributes, '' !== (string) self::resolveAnyLabel($item->getLabel())); + MFormLayoutCore::consumeCollapseWrapperAttributes($attributes);🤖 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 `@lib/MForm/Parser/MFormParser.php` around lines 277 - 301, The collapse handling in MFormParser::parseElement is still duplicating the wrapper-attribute cleanup instead of using the shared MFormLayoutCore::consumeCollapseWrapperAttributes helper. Replace the local hard-coded removal list for data-group-hide-toggle-links, data-group-accordion, and data-group-open-collapse with a call to that helper so the parser matches the existing MFormFlexRepeaterRenderer path and stays consistent if the keys change.
🤖 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 `@assets/css/mform.css`:
- Around line 615-624: Remove the empty line before the border declaration in
the section.repeater .repeater-group rule in mform.css to satisfy the
declaration-empty-line-before Stylelint rule; keep the declarations in the same
block together without a blank line between transition and border, and verify
the rest of the repeater-group styling remains unchanged.
In `@lib/MForm/Parser/MFormParser.php`:
- Around line 1710-1797: The helper logic here is duplicated across MFormParser,
MFormLabelRenderer, and MFormFlexRepeaterRenderer, so move the shared label/tab
helpers (resolveAnyLabel, isTabActive, isTabPullRight, buildTabNavClass,
isTabLayoutVertical, isTabStyleModern, isTruthyFlag) into MFormLayoutCore or the
existing shared renderer and have MFormParser reference that shared
implementation instead of keeping a local copy. Also add a PHPDoc block for
resolveAnyLabel to match the neighboring helper methods and keep the
PHPDoc/type-hint style consistent.
In `@lib/MForm/Template/MFormLabelRenderer.php`:
- Around line 41-45: The tooltip rendering in
MFormLabelRenderer::renderInfoTooltip currently injects getInfoTooltip() into
the title attribute and the icon value into the class attribute without
escaping. Update this block to pass both values through rex_escape() before
concatenation so quotes and other characters cannot break out of the attributes,
and keep the change localized to the info-tooltip anchor markup.
---
Nitpick comments:
In `@assets/css/mform.css`:
- Around line 5-192: The dark-theme variable block is duplicated in both the
body.rex-theme-dark rule and the `@media` (prefers-color-scheme: dark) override,
so keep the two sections in sync by extracting the shared declarations into a
single source of truth if the build supports it (for example via a preprocessor
mixin or shared variable block) and then reference that in both selectors; use
the duplicated CSS variable groups under .mform/.mfr-container as the target for
the cleanup.
In `@lib/MForm/FlexRepeater/MFormFlexRepeaterRenderer.php`:
- Around line 595-616: The label resolution logic in
getLabelString()/resolveAnyLabel() is duplicated from
MFormLabelRenderer::resolveLabelValue() and should be centralized. Replace the
local implementation in MFormFlexRepeaterRenderer with the shared
MFormLabelRenderer helper already available via the import, and keep MFormParser
aligned by reusing the same label resolution path so the behavior stays
consistent and avoids future divergence.
In `@lib/MForm/Parser/MFormParser.php`:
- Around line 277-301: The collapse handling in MFormParser::parseElement is
still duplicating the wrapper-attribute cleanup instead of using the shared
MFormLayoutCore::consumeCollapseWrapperAttributes helper. Replace the local
hard-coded removal list for data-group-hide-toggle-links, data-group-accordion,
and data-group-open-collapse with a call to that helper so the parser matches
the existing MFormFlexRepeaterRenderer path and stays consistent if the keys
change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 457d2fa1-c865-4a96-b21b-c5eb16cebd99
📒 Files selected for processing (16)
CHANGELOG.mdassets/css/mform.cssassets/mform.jsdocs/01_basics.mddocs/05_wrapper.mddocs/13_api_reference.mdfragments/mform/mform_wrapper.phplang/de_de.langlang/en_gb.langlib/MForm/FlexRepeater/MFormFlexRepeaterRenderer.phplib/MForm/Migration/MBlockToRepeaterConverter.phplib/MForm/Parser/MFormParser.phplib/MForm/Template/MFormLabelRenderer.phplib/MForm/Template/MFormLayoutCore.phppackage.ymlpages/demo.demo_renderer_parity.php
**Einfache Feldparität im Flex-Repeater erweitert** - `datetime` wird im Repeater-Template jetzt direkt gerendert, Textfelder unterstützen Datalist-Optionen analog zum Parser, und `markitup` wird bewusst als normale `textarea`-Fallback-Ausgabe behandelt.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/MForm/Parser/MFormParser.php (1)
279-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMoin – hier bietet sich der zentrale Helper für Parität an.
Die Literal-Liste in
$removeAttributesist identisch zu dem, wasMFormLayoutCore::consumeCollapseWrapperAttributes()entfernt – genau diesen Helper nutzt der Flex-Repeater bereits (MFormFlexRepeaterRenderer.php:148). Um die von dieser PR angestrebte Parität zu sichern und ein Auseinanderdriften der beiden Pfade zu vermeiden, sollte der Parser denselben Helper verwenden statt die Keys erneut hart zu kodieren.♻️ Vorschlag: Helper statt Literal-Liste
// COLLAPSE MANIPULATIONS if ('collapse' == $item->getType()) { - $removeAttributes = ['data-group-hide-toggle-links', 'data-group-accordion', 'data-group-open-collapse']; $openCollapse = MFormLayoutCore::isCollapseOpen($attributes); $isAccordion = MFormLayoutCore::isCollapseAccordion($attributes); @@ $element->setLabel($this->parseElement($collapseButton, 'wrapper')); // add parsed legend to collapse element + MFormLayoutCore::consumeCollapseWrapperAttributes($attributes); }🤖 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 `@lib/MForm/Parser/MFormParser.php` around lines 279 - 302, The collapse-parser path is duplicating the same wrapper-attribute removal logic as MFormLayoutCore::consumeCollapseWrapperAttributes(), which risks drift from the Flex-Repeater behavior. Update MFormParser::parseCollapse handling to use MFormLayoutCore::consumeCollapseWrapperAttributes() instead of maintaining the hard-coded $removeAttributes list, so both rendering paths share the same central helper and stay in parity.
🤖 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 `@lib/MForm/Migration/MBlockToRepeaterConverter.php`:
- Around line 592-600: Die Warn-Erkennung in collectInputWarnings() deckt nur
addMediaField(), addLinkField() und addCustomLinkField() ab, obwohl die
Normalisierung auch addMFormMediaField() und addMFormLinkField() verarbeitet.
Ergänze die bestehenden preg_match-Prüfungen in MBlockToRepeaterConverter so,
dass numerische addMFormMediaField(n, ...) und addMFormLinkField(n, ...)
ebenfalls als Warnung erfasst werden, mit passenden Hinweisen analog zu den
vorhandenen Meldungen.
---
Nitpick comments:
In `@lib/MForm/Parser/MFormParser.php`:
- Around line 279-302: The collapse-parser path is duplicating the same
wrapper-attribute removal logic as
MFormLayoutCore::consumeCollapseWrapperAttributes(), which risks drift from the
Flex-Repeater behavior. Update MFormParser::parseCollapse handling to use
MFormLayoutCore::consumeCollapseWrapperAttributes() instead of maintaining the
hard-coded $removeAttributes list, so both rendering paths share the same
central helper and stay in parity.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 037ac388-8cbf-42a1-a1f5-d47560a348b3
📒 Files selected for processing (10)
CHANGELOG.mdassets/css/mform.cssassets/mform.jslib/MForm/FlexRepeater/MFormFlexRepeaterRenderer.phplib/MForm/Migration/MBlockToRepeaterConverter.phplib/MForm/Parser/MFormParser.phplib/MForm/Template/MFormFieldTypeCore.phplib/MForm/Template/MFormLabelRenderer.phplib/MForm/Template/MFormLayoutCore.phppages/demo.demo_renderer_parity.php
💤 Files with no reviewable changes (1)
- assets/css/mform.css
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- assets/mform.js
- lib/MForm/Template/MFormLabelRenderer.php
- pages/demo.demo_renderer_parity.php
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
lib/MForm/Template/MFormLayoutCore.php (4)
66-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocblock unvollständig:
$itemClassfehlt im@param.Der Docblock dokumentiert nur
$attributes, der zweite Parameter$itemClass(mit Default-Wert) fehlt vollständig.📝 Vorschlag
/** * Ensures selectpicker fields have a stable default container. * * `@param` array<string, mixed> $attributes + * `@param` string $itemClass */ public static function ensureSelectpickerContainer(array &$attributes, string $itemClass = ''): voidAs per coding guidelines, "Keep PHPDoc and type hints clean and consistent in PHP code".
🤖 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 `@lib/MForm/Template/MFormLayoutCore.php` around lines 66 - 95, The PHPDoc for ensureSelectpickerContainer is missing documentation for the $itemClass parameter, so update the docblock to include a matching `@param` entry for the second argument alongside $attributes. Keep the comment consistent with the method signature and existing type hints in MFormLayoutCore.Source: Coding guidelines
30-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueToter Code:
true === $value-Zweig ist unerreichbar.
(int) trueergibt1, also erfüllt ein booleschertrue-Wert bereits die erste Bedingung1 === (int) $attributes[...]. Die zweite Alternativetrue === $attributes[...]wird dadurch nie exklusiv ausgewertet – sie ist redundant.🤖 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 `@lib/MForm/Template/MFormLayoutCore.php` around lines 30 - 34, The isCollapseOpen method in MFormLayoutCore has a redundant true comparison because a boolean true value already satisfies the integer check, making the second branch unreachable. Simplify the condition so it only checks the normalized attribute value once, and keep the logic centered on data-group-open-collapse within isCollapseOpen.
30-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInkonsistente Wahrheitswert-Prüfung zwischen Collapse- und Tab-Flags.
isCollapseOpen/isCollapseAccordionakzeptieren nur strikt1/true, währendshouldHideCollapseTogglenur den String'true'akzeptiert – aber keines der drei nutzt die eigens dafür gebauteisTruthyFlag()-Hilfsmethode (Zeile 177), die für die Tab-Flags (isTabActive,isTabPullRight)'1','true'undtruegleichermaßen behandelt. Wirddata-group-open-collapseals String"1"oder"true"übergeben (z. B. aus HTML-Attributen), schlägtisCollapseOpen()fehl, obwohl das Pendant bei Tabs funktionieren würde. Downstream im Parser (data-collapse-open' => $openCollapse ? 1 : 0) zeigt zudem, dass hier eher mit Integer-Flags gearbeitet wird – ein String'true'würde also nirgends konsequent unterstützt.Für konsistentes Verhalten
isTruthyFlag()auch für die Collapse-Helfer verwenden.♻️ Vorschlag zur Vereinheitlichung
public static function isCollapseOpen(array $attributes): bool { - return isset($attributes['data-group-open-collapse']) - && (1 === (int) $attributes['data-group-open-collapse'] || true === $attributes['data-group-open-collapse']); + return isset($attributes['data-group-open-collapse']) + && self::isTruthyFlag($attributes['data-group-open-collapse']); } public static function isCollapseAccordion(array $attributes): bool { - return isset($attributes['data-group-accordion']) && 1 === (int) $attributes['data-group-accordion']; + return isset($attributes['data-group-accordion']) && self::isTruthyFlag($attributes['data-group-accordion']); } public static function shouldHideCollapseToggle(array $attributes, bool $hasLabel): bool { return !$hasLabel || (array_key_exists('data-group-hide-toggle-links', $attributes) - && 'true' === (string) $attributes['data-group-hide-toggle-links']); + && self::isTruthyFlag($attributes['data-group-hide-toggle-links'])); }🤖 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 `@lib/MForm/Template/MFormLayoutCore.php` around lines 30 - 52, The Collapse flag helpers in MFormLayoutCore are using inconsistent boolean checks compared to the Tab helpers and should be unified. Update isCollapseOpen, isCollapseAccordion, and shouldHideCollapseToggle to use the existing isTruthyFlag() helper (like isTabActive and isTabPullRight) so values such as "1", "true", and true are handled consistently for data-group-open-collapse, data-group-accordion, and data-group-hide-toggle-links.
157-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFehlendes Docblock bei
isTruthyFlag.
consumeClassAttributesdirekt darüber hat ein vollständiges Docblock mit@param-Angaben,isTruthyFlaghingegen gar keins, obwohl es einenmixed-Parameter besitzt, dessen erwartete Werte (bool/string/numeric) dokumentationswürdig wären.As per coding guidelines, "Keep PHPDoc and type hints clean and consistent in PHP code".
🤖 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 `@lib/MForm/Template/MFormLayoutCore.php` around lines 157 - 189, `isTruthyFlag` is missing the same PHPDoc treatment used by `consumeClassAttributes`, so add a concise docblock above `MFormLayoutCore::isTruthyFlag` documenting the `mixed` input and its accepted truthy forms (bool, string, numeric) to keep PHPDoc and type hints consistent. Make sure the comment matches the method’s behavior and style used elsewhere in the class.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@lib/MForm/Template/MFormLayoutCore.php`:
- Around line 66-95: The PHPDoc for ensureSelectpickerContainer is missing
documentation for the $itemClass parameter, so update the docblock to include a
matching `@param` entry for the second argument alongside $attributes. Keep the
comment consistent with the method signature and existing type hints in
MFormLayoutCore.
- Around line 30-34: The isCollapseOpen method in MFormLayoutCore has a
redundant true comparison because a boolean true value already satisfies the
integer check, making the second branch unreachable. Simplify the condition so
it only checks the normalized attribute value once, and keep the logic centered
on data-group-open-collapse within isCollapseOpen.
- Around line 30-52: The Collapse flag helpers in MFormLayoutCore are using
inconsistent boolean checks compared to the Tab helpers and should be unified.
Update isCollapseOpen, isCollapseAccordion, and shouldHideCollapseToggle to use
the existing isTruthyFlag() helper (like isTabActive and isTabPullRight) so
values such as "1", "true", and true are handled consistently for
data-group-open-collapse, data-group-accordion, and
data-group-hide-toggle-links.
- Around line 157-189: `isTruthyFlag` is missing the same PHPDoc treatment used
by `consumeClassAttributes`, so add a concise docblock above
`MFormLayoutCore::isTruthyFlag` documenting the `mixed` input and its accepted
truthy forms (bool, string, numeric) to keep PHPDoc and type hints consistent.
Make sure the comment matches the method’s behavior and style used elsewhere in
the class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7e31e45-f70d-4cc9-acf3-1796577a4bb1
📒 Files selected for processing (4)
assets/mform.jslib/MForm/FlexRepeater/MFormFlexRepeaterRenderer.phplib/MForm/Parser/MFormParser.phplib/MForm/Template/MFormLayoutCore.php
💤 Files with no reviewable changes (1)
- assets/mform.js
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/MForm/Parser/MFormParser.php
- lib/MForm/FlexRepeater/MFormFlexRepeaterRenderer.php
Contributor
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 `@assets/js/flex-repeater.js`:
- Around line 714-715: The mfr:clipboard-updated document listener added in
MFormFlexRepeater should be cleaned up during teardown to avoid leaving handlers
behind when an instance is removed. Add the matching removeEventListener call in
the same class where _onClipboardUpdated is registered, using the existing
MFormFlexRepeater teardown/dispose path so the callback is detached before the
instance is discarded.
- Around line 1232-1284: The clipboard state in _setClipboardSnapshot,
_getClipboardSnapshot, and _clearClipboard is currently shared globally through
mfr_clipboard and window._mfrClipboard, which causes different copyPaste
repeater instances to affect each other. Scope the clipboard per repeater
instance by deriving the storage key from a unique identifier such as fieldName
or the template/repeater context, and make _updateClipboardUi and the related
paste logic read/write only that instance-specific snapshot.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 110777db-8fb5-43cf-806f-40fd8493020a
📒 Files selected for processing (6)
CHANGELOG.mdassets/css/flex-repeater.cssassets/js/flex-repeater.jslang/de_de.langlang/en_gb.langlib/MForm/Parser/MFormParser.php
✅ Files skipped from review due to trivial changes (3)
- lang/de_de.lang
- lang/en_gb.lang
- CHANGELOG.md
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
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.
Behoben
addColumnElement()wird jetzt in allen relevanten Renderpfaden konsistent inrow-Gruppen geführt. Damit ist kein manueller HTML-Workaround mitaddHtml('<div class="row">')mehr nötig.row form-group(klassischer Parser und Flex-Repeater), damitcol-*-Spalten erwartungsgemäß funktionieren.setTooltipInfo()im Flex-Repeater funktionsgleich zum Parser - Label- und Tooltip-Rendering laufen jetzt über einen gemeinsamen internen Renderer. Dadurch wird der Tooltip im Repeater nicht mehr ignoriert und die Label-Aufbereitung (inkl. Sprach-Array-Fallback) bleibt über beide Pfade konsistent.setTooltipInfo()kein eigenes Icon übergeben wird, verwendet MForm jetzt standardmäßigfa-info-circlestattfa-exclamation.data-group-column-row-class/data-group-row-class(Columns) sowiedata-modal-row-class/data-group-row-class(Modal) wurde in einen gemeinsamen internen Core ausgelagert und wird jetzt von Parser und Flex-Repeater genutzt.open/accordion/hide-toggle-linksund die Bereinigung der Collapse-Wrapper-Attribute laufen jetzt über einen gemeinsamen Layout-Core und werden in Parser sowie Flex-Repeater gleich genutzt.vertical/modern) sowie die Bereinigung tab-spezifischer Meta-Attribute sind jetzt in Parser und Flex-Repeater konsistent umgesetzt (inkl. robuster Truthy-Auswertung fürtrue/1)..selectpickerkein explizitesdata-containergesetzt ist, wird beim Initialisieren standardmäßigbodyverwendet. Das verhindert falsch angedockte Dropdowns in verschachtelten Wrappern/Reapeatern.Neu
demo_renderer_parityzur Gegenprobe von Parser-HTML und Flex-Repeater-Template-HTML inkl. Marker-Checks für Tooltip, Row-/Modal-Klassen und Full-Layout.data-group-column-row-class(Alias:data-group-row-class) gesetzt werden.data-modal-row-class(Alias:data-group-row-class) möglich.Summary by CodeRabbit
datetime, Datalist,markitup-Fallback,password).fa-info-circle.data-container="body".