Relation controller popup: fix size, add cssClass and allowDismiss - #1044
Relation controller popup: fix size, add cssClass and allowDismiss#1044AIC-BV wants to merge 18 commits into
Conversation
mjauvin
added
enhancement
|
@LukeTowers Should be finished |
|
Could add allowDismiss (see comments) |
AIC-BV
changed the title
AIC-BV
changed the title
AIC-BV
changed the title
|
@AIC-BV just a thought, is there a way we could integrate with the change monitor used in the cms section for the tabs to detect when a change has happened inside of a modal and warn / prevent the dismiss behaviour from being triggered in those cases? |
I assume something like this could be a basic for that kind of behaviour if (this.options.allowDismiss) {
modal.on('click', function(e) {
const target = e.target;
if (target.classList.contains('control-popup')) {
if (!modal.dataset.hasChanges) {
modal.hide()
$('.popup-backdrop').remove()
$(document.body).removeClass('modal-open')
} else {
// popup warning them about unsaved changes
}
}
});
const inputs = modal.querySelectorAll('input')
for (const input of inputs) {
input.addEventListener('change', function() {
modal.dataset.hasChanges = true
})
}
} |
|
Thoughts @LukeTowers @mjauvin ? |
|
@AIC-BV seems fine, have you tested it out? |
|
I'm using everything except the latest comment: #1044 (comment) |
|
@AIC-BV can we remove allowDismiss from this PR for now then? I don't want to support it unless it can be a bit safer first. |
|
If its ok for you I will add the popup shortly instead |
Can you help me getting started? Also, will this be enough? PS: |
|
Ran composer update and this got overwritten ofcourse. |
|
@AIC-BV waiting on the docs PR to get updated / my comment to be responded to: https://github.com/wintercms/docs/pull/207/files#r1739737060. Will also need to update from develop and recompile the JS. |
Oops 👀 |
|
@AIC-BV are we good with all the requested changes? I'd like this to get merged. |
I think there is this pending feedback but I don't know what to do with it wintercms/docs#207 |
@LukeTowers could you clarify so that can be completed? |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe relation controller now propagates popup size, custom CSS classes, and backdrop dismissal settings to Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @modules/system/assets/ui/js/popup.js:
- Around line 218-227: The allowDismiss mousedown handler bypasses the Popup
lifecycle by calling jQuery's modal.hide() and manually removing backdrop/body
classes; replace that handler so it calls the Popup instance's hide() method
(use this.hide() bound to the popup context or call popup.hide()), remove manual
$('.popup-backdrop').remove() and $(document.body).removeClass('modal-open')
calls so Bootstrap/Popup cleanup and hide events run, and ensure the hide flow
honors the allowHide/lock checks (use this.lock(true) or check current lock
state before dismissing). If you prefer the safer option requested in PR, remove
or disable the allowDismiss feature entirely in this patch instead of
re-enabling it until InputMonitor (or equivalent unsaved-changes detection) is
integrated.
In @modules/system/assets/ui/storm-min.js:
- Around line 2102-2104: Remove the temporary allowDismiss implementation from
this PR by deleting the conditional block that attaches the mousedown handler
(the code using this.options.allowDismiss and modal.on('mousedown', ...)), and
remove any direct DOM manipulations inside it (the $('.popup-backdrop').remove()
and $(document.body).removeClass('modal-open') calls); leave Popup's existing
hide/dispose behavior untouched (do not add new event listeners or direct
backdrop/body manipulation here). If any configuration/defaults or docs were
added for allowDismiss, remove those as well. If reintroducing the feature
later, attach the handler to an instance property, check popup lock/state and
unsaved-change detection before calling modal.hide(), and remove the handler in
Popup.prototype.dispose rather than manipulating backdrop/body directly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/backend/behaviors/RelationController.phpmodules/backend/behaviors/relationcontroller/assets/js/winter.relation.jsmodules/system/assets/ui/js/popup.jsmodules/system/assets/ui/storm-min.js
🧰 Additional context used
🧬 Code graph analysis (3)
modules/backend/behaviors/relationcontroller/assets/js/winter.relation.js (1)
modules/backend/classes/FormField.php (1)
size(217-221)
modules/system/assets/ui/js/popup.js (1)
modules/system/assets/ui/storm-min.js (1)
target(2102-2102)
modules/backend/behaviors/RelationController.php (1)
modules/backend/classes/FormField.php (1)
size(217-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: ubuntu-latest / PHP 8.2
- GitHub Check: ubuntu-latest / PHP 8.3
- GitHub Check: ubuntu-latest / PHP 8.1
- GitHub Check: windows-latest / JavaScript
🔇 Additional comments (9)
modules/backend/behaviors/RelationController.php (2)
688-690: LGTM: Config properties correctly retrieved.The three new configuration properties (size, cssClass, allowDismiss) are properly retrieved from the manage section of the relation config. These values default to null if not specified, which is handled appropriately by the downstream JavaScript code.
692-700: LGTM: Parameter passing is consistent.The sprintf format string and arguments are correctly aligned. The six parameters match the updated JavaScript function signature in winter.relation.js, enabling the popup configuration to flow from the backend to the frontend.
modules/system/assets/ui/js/popup.js (2)
52-54: LGTM: Safe default values.The new default values are appropriately conservative. Setting allowDismiss to false by default ensures existing popups maintain their current behavior and only opt-in to the new dismiss functionality.
212-213: LGTM: cssClass implementation is safe.The cssClass is correctly applied to the modal-dialog element. jQuery's addClass method safely handles the input without XSS risk.
modules/backend/behaviors/relationcontroller/assets/js/winter.relation.js (2)
12-12: LGTM: Function signature extended correctly.The updated signature accepts the three new parameters (size, cssClass, allowDismiss) that are now passed from the PHP backend. This maintains consistency across the backend-to-frontend data flow.
17-25: The code correctly uses 'huge' as a valid popup size with appropriate CSS styling.The default size of 'huge' is valid and well-established in the codebase, with CSS class definitions present in multiple stylesheet files. The popup configuration correctly passes all three parameters (size, cssClass, allowDismiss) to the popup plugin.
modules/system/assets/ui/storm-min.js (3)
2100-2100: LGTM: cssClass implementation looks correct.The cssClass option is properly applied to the modal dialog element, allowing custom styling as documented in the PR objectives.
4683-4683: Clarify unrelated change to listSortable.This change to the listSortable component appears unrelated to the PR's objectives (relation controller popup configuration). Is this an unintentional side effect from recompiling the minified file, or does it serve a purpose for this PR?
If unintentional, consider reverting this change to keep the PR focused on relation popup enhancements.
Could you confirm whether this change was intentional and how it relates to the popup size/cssClass/allowDismiss features?
2056-2056: Consider disabling or enhancing allowDismiss with change detection.The
allowDismissimplementation allows users to dismiss popups by clicking the backdrop without validating for unsaved changes. While the feature defaults tofalse(opt-in), if enabled it should integrate with the Input Monitor system to prevent accidental data loss.The
cssClassaddition is good and can remain.Suggested improvements if allowDismiss is kept:
- Integrate with the Input Monitor (input.monitor.js) to detect unsaved changes before allowing dismissal
- Show a confirmation dialog if changes are detected
- Clean up the
mousedownevent listener on popup disposal to prevent memory leaks
| if (this.options.allowDismiss) { | ||
| modal.on('mousedown', function(e) { | ||
| const target = e.target; | ||
| if (target.classList.contains('control-popup')) { | ||
| modal.hide() | ||
| $('.popup-backdrop').remove() | ||
| $(document.body).removeClass('modal-open') | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Critical: allowDismiss implementation bypasses popup lifecycle and lock mechanism.
The current implementation has several critical flaws:
-
Line 222 uses
modal.hide()(jQuery's hide method) instead of calling the Popup instance'shide()method (line 334). This bypasses:- The
allowHidelock check (line 340) which prevents closing locked popups - The hide event triggers (lines 337-338)
- The proper cleanup flow managed by Bootstrap modal events
- The
-
Lines 223-224 manually remove backdrop and body class instead of letting the Bootstrap modal hide events handle cleanup (lines 148-154). This can leave the popup in an inconsistent state.
-
Missing unsaved changes protection: Per PR discussion, there is no change monitoring to warn users about unsaved data before dismissing. This creates a risk of accidental data loss.
Recommendation: Based on the PR discussion where LukeTowers requested removing allowDismiss until safer handling is implemented, consider either:
- Remove the allowDismiss feature from this PR
- Integrate Winter UI Input Monitor to detect unsaved changes before allowing dismiss
- Use the popup's lock mechanism (
this.lock(true)) when changes are detected
🔧 Proposed fix to use proper popup hide method
if (this.options.allowDismiss) {
+ var self = this
modal.on('mousedown', function(e) {
const target = e.target;
if (target.classList.contains('control-popup')) {
- modal.hide()
- $('.popup-backdrop').remove()
- $(document.body).removeClass('modal-open')
+ // Use the popup instance's hide method to ensure proper cleanup
+ self.hide()
}
});
}Note: This fix addresses the lifecycle bypass but does not solve the missing change monitoring concern.
🤖 Prompt for AI Agents
In @modules/system/assets/ui/js/popup.js around lines 218 - 227, The
allowDismiss mousedown handler bypasses the Popup lifecycle by calling jQuery's
modal.hide() and manually removing backdrop/body classes; replace that handler
so it calls the Popup instance's hide() method (use this.hide() bound to the
popup context or call popup.hide()), remove manual $('.popup-backdrop').remove()
and $(document.body).removeClass('modal-open') calls so Bootstrap/Popup cleanup
and hide events run, and ensure the hide flow honors the allowHide/lock checks
(use this.lock(true) or check current lock state before dismissing). If you
prefer the safer option requested in PR, remove or disable the allowDismiss
feature entirely in this patch instead of re-enabling it until InputMonitor (or
equivalent unsaved-changes detection) is integrated.
| if(this.options.allowDismiss){modal.on('mousedown',function(e){const target=e.target;if(target.classList.contains('control-popup')){modal.hide() | ||
| $('.popup-backdrop').remove() | ||
| $(document.body).removeClass('modal-open')}});}return modal.append(modalDialog.append(modalContent))} |
There was a problem hiding this comment.
Critical: allowDismiss implementation lacks change detection and proper cleanup.
Multiple concerns with the current allowDismiss implementation:
- Data loss risk: No validation for unsaved changes before closing (the primary concern raised in PR discussion)
- Memory leak: The mousedown event listener is never removed when the popup is disposed
- Direct DOM manipulation: Directly manipulating backdrop and body classes may conflict with the Popup's internal state management
The event listener should be stored and removed in the dispose method. However, per maintainer feedback, the entire allowDismiss feature should be removed from this PR until proper change-monitoring integration is complete.
🐛 Issues to address if keeping this feature
If allowDismiss is kept (against maintainer's request), at minimum:
- Store the event handler reference and remove it in
Popup.prototype.dispose - Check if the popup is locked before allowing dismissal
- Integrate change detection to prevent data loss
- Use the Popup's own hide method logic rather than direct DOM manipulation
However, the recommended action is to remove this feature entirely until proper change-monitoring is implemented.
Based on PR discussion where LukeTowers requested removal of allowDismiss until safer implementation.
🤖 Prompt for AI Agents
In @modules/system/assets/ui/storm-min.js around lines 2102 - 2104, Remove the
temporary allowDismiss implementation from this PR by deleting the conditional
block that attaches the mousedown handler (the code using
this.options.allowDismiss and modal.on('mousedown', ...)), and remove any direct
DOM manipulations inside it (the $('.popup-backdrop').remove() and
$(document.body).removeClass('modal-open') calls); leave Popup's existing
hide/dispose behavior untouched (do not add new event listeners or direct
backdrop/body manipulation here). If any configuration/defaults or docs were
added for allowDismiss, remove those as well. If reintroducing the feature
later, attach the handler to an instance property, check popup lock/state and
unsaved-change detection before calling modal.hide(), and remove the handler in
Popup.prototype.dispose rather than manipulating backdrop/body directly.
|
Do I still need to do anything for this PR? |
- Route backdrop dismissal through the popup's own hide() method so the Bootstrap modal lifecycle, hide/hidden events, lock() and disposal all run as normal, instead of manually hiding the modal and removing every backdrop on the page (which broke stacked popups and leaked instances) - Keep the popup open (with a shake) when it contains unsaved changes tracked by the change monitor (data-change-monitor) - Rename the relation config key cssClass to popupClass as requested - Pass the popup options from RelationController as json_encoded values so booleans/null arrive typed in JS, and default the size via getConfig - Document data-css-class and data-allow-dismiss in the popup UI docs - Recompile storm-min.js via winter:util compile js Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Attach the change monitor (data-change-monitor) to the relation manage popup forms when allowDismiss is enabled, so unsaved changes inside the popup block backdrop dismissal by default - Show a translatable warning flash message alongside the shake when dismissal is refused because of unsaved changes - Add the popup.unsaved_changes client lang key (en, nl) and recompile the client-side language files - Recompile storm-min.js Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@LukeTowers I let Claude tackle your concerns |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@modules/system/assets/js/lang/lang.be.js`:
- Line 9: Update the markdowneditor.horizontalrule translation in the language
resource to use the complete label “Insert Horizontal Rule” or a suitable
Belarusian translation, preserving the surrounding translation entries.
In `@modules/system/assets/js/lang/lang.et.js`:
- Line 9: Replace the English popup.unsaved_changes value with the appropriate
locale-specific translation in modules/system/assets/js/lang/lang.et.js#L9-L9,
lang.fa.js#L9-L9, lang.fi.js#L9-L9, lang.fr-ca.js#L9-L9, lang.fr.js#L9-L9,
lang.hu.js#L9-L9, lang.id.js#L9-L9, lang.it.js#L9-L9, lang.ja.js#L9-L9,
lang.kr.js#L9-L9, and lang.lt.js#L9-L9, then regenerate the compiled JavaScript
language bundles from the source catalogs.
In `@modules/system/assets/js/lang/lang.nl.js`:
- Line 9: Update the datepicker.nextMonth translation in the Dutch language
resource from “Volgende maan” to “Volgende maand”, leaving the other datepicker
labels unchanged.
In `@modules/system/assets/js/lang/lang.pl.js`:
- Line 9: Remove the stray escaped quote character from the start of the
unorderedlist value in the markdowneditor section. The unorderedlist key
currently has an unwanted leading quote that will display in the UI; update the
value to start directly with "Lista" instead of with the escaped quote
character.
In `@modules/system/assets/js/lang/lang.pt-br.js`:
- Line 9: Correct the typo in filter.numbers.filter_button_text from “Filtar” to
“Filtrar”, leaving the surrounding Portuguese filter translations unchanged.
In `@modules/system/assets/js/lang/lang.zh-cn.js`:
- Line 9: Restore filter.numbers.all in the Simplified Chinese translation to
the original Chinese value “全部” instead of “all”. Preserve the existing
popup.unsaved_changes entry and all other regenerated translations unchanged.
🪄 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 Plus
Run ID: 83956a88-3ec8-4618-96e8-40805de66bdd
📒 Files selected for processing (50)
modules/backend/behaviors/RelationController.phpmodules/backend/behaviors/relationcontroller/partials/_manage_form.phpmodules/system/assets/js/lang/lang.ar.jsmodules/system/assets/js/lang/lang.be.jsmodules/system/assets/js/lang/lang.bg.jsmodules/system/assets/js/lang/lang.ca.jsmodules/system/assets/js/lang/lang.cs.jsmodules/system/assets/js/lang/lang.da.jsmodules/system/assets/js/lang/lang.de.jsmodules/system/assets/js/lang/lang.el.jsmodules/system/assets/js/lang/lang.en-au.jsmodules/system/assets/js/lang/lang.en-ca.jsmodules/system/assets/js/lang/lang.en-gb.jsmodules/system/assets/js/lang/lang.en.jsmodules/system/assets/js/lang/lang.es-ar.jsmodules/system/assets/js/lang/lang.es.jsmodules/system/assets/js/lang/lang.et.jsmodules/system/assets/js/lang/lang.fa.jsmodules/system/assets/js/lang/lang.fi.jsmodules/system/assets/js/lang/lang.fr-ca.jsmodules/system/assets/js/lang/lang.fr.jsmodules/system/assets/js/lang/lang.hu.jsmodules/system/assets/js/lang/lang.id.jsmodules/system/assets/js/lang/lang.it.jsmodules/system/assets/js/lang/lang.ja.jsmodules/system/assets/js/lang/lang.kr.jsmodules/system/assets/js/lang/lang.lt.jsmodules/system/assets/js/lang/lang.lv.jsmodules/system/assets/js/lang/lang.nb-no.jsmodules/system/assets/js/lang/lang.nl.jsmodules/system/assets/js/lang/lang.pl.jsmodules/system/assets/js/lang/lang.pt-br.jsmodules/system/assets/js/lang/lang.pt-pt.jsmodules/system/assets/js/lang/lang.ro.jsmodules/system/assets/js/lang/lang.rs.jsmodules/system/assets/js/lang/lang.ru.jsmodules/system/assets/js/lang/lang.sk.jsmodules/system/assets/js/lang/lang.sl.jsmodules/system/assets/js/lang/lang.sv.jsmodules/system/assets/js/lang/lang.th.jsmodules/system/assets/js/lang/lang.tr.jsmodules/system/assets/js/lang/lang.uk.jsmodules/system/assets/js/lang/lang.vn.jsmodules/system/assets/js/lang/lang.zh-cn.jsmodules/system/assets/js/lang/lang.zh-tw.jsmodules/system/assets/ui/docs/popup.mdmodules/system/assets/ui/js/popup.jsmodules/system/assets/ui/storm-min.jsmodules/system/lang/en/client.phpmodules/system/lang/nl/client.php
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/backend/behaviors/RelationController.php
- modules/system/assets/ui/storm-min.js
- modules/system/assets/ui/js/popup.js
| $.wn.langMessages['et'] = $.extend( | ||
| $.wn.langMessages['et'] || {}, | ||
| {"markdowneditor":{"formatting":"Vorming","quote":"Tsitaat","code":"Kood","header1":"Pealkiri 1","header2":"Pealkiri 2","header3":"Pealkiri 3","header4":"Pealkiri 4","header5":"Pealkiri 5","header6":"Pealkiri 6","bold":"Paks","italic":"Kursiiv","unorderedlist":"J\u00e4rjestamata nimekiri","orderedlist":"J\u00e4rjestatud nimekiri","video":"Video","image":"Pilt","link":"Link","horizontalrule":"Sisesta horisontaaljoon","fullscreen":"T\u00e4isekraan","preview":"Eelvaade"},"mediamanager":{"insert_link":"Sisesta link","insert_image":"Siseta pilt","insert_video":"Sisesta video","insert_audio":"Sisesta heliklipp","invalid_file_empty_insert":"Palun vali fail, millele link lisada.","invalid_file_single_insert":"Palun vali \u00fcks fail.","invalid_image_empty_insert":"Palun vali pildid, mida lisada.","invalid_video_empty_insert":"Palun vali videoklipp, mida lisada.","invalid_audio_empty_insert":"Palun vali heliklipp, mida lisada."},"alert":{"confirm_button_text":"OK","cancel_button_text":"Loobu","widget_remove_confirm":"Eemalda see widget?"},"datepicker":{"previousMonth":"Eelmine kuu","nextMonth":"J\u00e4rgmine kuu","months":["Jaanuar","Veebruar","M\u00e4rts","Aprill","Mai","Juuni","Juuli","August","September","Oktoober","November","Detsember"],"weekdays":["P\u00fchap\u00e4ev","Esmasp\u00e4ev","Teisip\u00e4ev","Kolmap\u00e4ev","Neljap\u00e4ev","Reede","Laup\u00e4ev"],"weekdaysShort":["P","E","T","K","N","R","L"]},"colorpicker":{"last_color":"Use previously selected color","aria_palette":"Color selection area","aria_hue":"Hue selection slider","aria_opacity":"Opacity selection slider"},"filter":{"group":{"all":"k\u00f5ik"},"scopes":{"apply_button_text":"Apply","clear_button_text":"Clear"},"dates":{"all":"k\u00f5ik","filter_button_text":"Filtreeri","reset_button_text":"L\u00e4htesta","date_placeholder":"Kuup\u00e4ev","after_placeholder":"Hiljem kui","before_placeholder":"Varem kui"},"numbers":{"all":"all","filter_button_text":"Filter","reset_button_text":"Reset","min_placeholder":"Min","max_placeholder":"Max"}},"eventlog":{"show_stacktrace":"N\u00e4ita stacktrace","hide_stacktrace":"Peida stacktrace","tabs":{"formatted":"Kujundatud","raw":"Algne"},"editor":{"title":"L\u00e4htekoodi redaktor","description":"Sinu operatsioonis\u00fcsteem peaks olema sedistatud \u00fche URL skeemi jaoks.","openWith":"Ava programmiga","remember_choice":"J\u00e4ta valik selleks sessiooniks meelde","open":"Ava","cancel":"Loobu"}}} | ||
| {"markdowneditor":{"formatting":"Vorming","quote":"Tsitaat","code":"Kood","header1":"Pealkiri 1","header2":"Pealkiri 2","header3":"Pealkiri 3","header4":"Pealkiri 4","header5":"Pealkiri 5","header6":"Pealkiri 6","bold":"Paks","italic":"Kursiiv","unorderedlist":"J\u00e4rjestamata nimekiri","orderedlist":"J\u00e4rjestatud nimekiri","video":"Video","image":"Pilt","link":"Link","horizontalrule":"Sisesta horisontaaljoon","fullscreen":"T\u00e4isekraan","preview":"Eelvaade"},"mediamanager":{"insert_link":"Sisesta link","insert_image":"Siseta pilt","insert_video":"Sisesta video","insert_audio":"Sisesta heliklipp","invalid_file_empty_insert":"Palun vali fail, millele link lisada.","invalid_file_single_insert":"Palun vali \u00fcks fail.","invalid_image_empty_insert":"Palun vali pildid, mida lisada.","invalid_video_empty_insert":"Palun vali videoklipp, mida lisada.","invalid_audio_empty_insert":"Palun vali heliklipp, mida lisada."},"alert":{"confirm_button_text":"OK","cancel_button_text":"Loobu","widget_remove_confirm":"Eemalda see widget?"},"popup":{"unsaved_changes":"The popup contains unsaved changes."},"datepicker":{"previousMonth":"Eelmine kuu","nextMonth":"J\u00e4rgmine kuu","months":["Jaanuar","Veebruar","M\u00e4rts","Aprill","Mai","Juuni","Juuli","August","September","Oktoober","November","Detsember"],"weekdays":["P\u00fchap\u00e4ev","Esmasp\u00e4ev","Teisip\u00e4ev","Kolmap\u00e4ev","Neljap\u00e4ev","Reede","Laup\u00e4ev"],"weekdaysShort":["P","E","T","K","N","R","L"]},"colorpicker":{"last_color":"Use previously selected color","aria_palette":"Color selection area","aria_hue":"Hue selection slider","aria_opacity":"Opacity selection slider"},"filter":{"group":{"all":"k\u00f5ik"},"scopes":{"apply_button_text":"Apply","clear_button_text":"Clear"},"dates":{"all":"k\u00f5ik","filter_button_text":"Filtreeri","reset_button_text":"L\u00e4htesta","date_placeholder":"Kuup\u00e4ev","after_placeholder":"Hiljem kui","before_placeholder":"Varem kui"},"numbers":{"all":"all","filter_button_text":"Filter","reset_button_text":"Reset","min_placeholder":"Min","max_placeholder":"Max"}},"eventlog":{"show_stacktrace":"N\u00e4ita stacktrace","hide_stacktrace":"Peida stacktrace","tabs":{"formatted":"Kujundatud","raw":"Algne"},"editor":{"title":"L\u00e4htekoodi redaktor","description":"Sinu operatsioonis\u00fcsteem peaks olema sedistatud \u00fche URL skeemi jaoks.","openWith":"Ava programmiga","remember_choice":"J\u00e4ta valik selleks sessiooniks meelde","open":"Ava","cancel":"Loobu"}}} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the new unsaved-changes warning in every locale bundle.
The same English message was added to all supplied locale files, so non-English users will receive an untranslated warning. Add locale-specific values in the source catalogs, then regenerate the compiled JavaScript bundles.
modules/system/assets/js/lang/lang.et.js#L9-L9: add the Estonian translation.modules/system/assets/js/lang/lang.fa.js#L9-L9: add the Persian translation.modules/system/assets/js/lang/lang.fi.js#L9-L9: add the Finnish translation.modules/system/assets/js/lang/lang.fr-ca.js#L9-L9: add the French Canadian translation.modules/system/assets/js/lang/lang.fr.js#L9-L9: add the French translation.modules/system/assets/js/lang/lang.hu.js#L9-L9: add the Hungarian translation.modules/system/assets/js/lang/lang.id.js#L9-L9: add the Indonesian translation.modules/system/assets/js/lang/lang.it.js#L9-L9: add the Italian translation.modules/system/assets/js/lang/lang.ja.js#L9-L9: add the Japanese translation.modules/system/assets/js/lang/lang.kr.js#L9-L9: add the Korean translation.modules/system/assets/js/lang/lang.lt.js#L9-L9: add the Lithuanian translation.
📍 Affects 11 files
modules/system/assets/js/lang/lang.et.js#L9-L9(this comment)modules/system/assets/js/lang/lang.fa.js#L9-L9modules/system/assets/js/lang/lang.fi.js#L9-L9modules/system/assets/js/lang/lang.fr-ca.js#L9-L9modules/system/assets/js/lang/lang.fr.js#L9-L9modules/system/assets/js/lang/lang.hu.js#L9-L9modules/system/assets/js/lang/lang.id.js#L9-L9modules/system/assets/js/lang/lang.it.js#L9-L9modules/system/assets/js/lang/lang.ja.js#L9-L9modules/system/assets/js/lang/lang.kr.js#L9-L9modules/system/assets/js/lang/lang.lt.js#L9-L9
🤖 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 `@modules/system/assets/js/lang/lang.et.js` at line 9, Replace the English
popup.unsaved_changes value with the appropriate locale-specific translation in
modules/system/assets/js/lang/lang.et.js#L9-L9, lang.fa.js#L9-L9,
lang.fi.js#L9-L9, lang.fr-ca.js#L9-L9, lang.fr.js#L9-L9, lang.hu.js#L9-L9,
lang.id.js#L9-L9, lang.it.js#L9-L9, lang.ja.js#L9-L9, lang.kr.js#L9-L9, and
lang.lt.js#L9-L9, then regenerate the compiled JavaScript language bundles from
the source catalogs.
| $.wn.langMessages['zh-cn'] = $.extend( | ||
| $.wn.langMessages['zh-cn'] || {}, | ||
| {"markdowneditor":{"formatting":"\u683c\u5f0f\u5316","quote":"\u5f15\u7528","code":"\u4ee3\u7801","header1":"\u6807\u9898 1","header2":"\u6807\u9898 2","header3":"\u6807\u9898 3","header4":"\u6807\u9898 4","header5":"\u6807\u9898 5","header6":"\u6807\u9898 6","bold":"\u7c97\u4f53","italic":"\u659c\u4f53","unorderedlist":"\u65e0\u5e8f\u5217\u8868","orderedlist":"\u6709\u5e8f\u5217\u8868","video":"\u89c6\u9891","image":"\u56fe\u7247","link":"\u94fe\u63a5","horizontalrule":"\u63d2\u5165\u5206\u5272\u7ebf","fullscreen":"\u5168\u5c4f","preview":"\u9884\u89c8"},"mediamanager":{"insert_link":"\u63d2\u5165\u94fe\u63a5","insert_image":"\u63d2\u5165\u56fe\u7247","insert_video":"\u63d2\u5165\u89c6\u9891","insert_audio":"\u63d2\u5165\u97f3\u9891","invalid_file_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u6587\u4ef6\u3002","invalid_file_single_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u6587\u4ef6\u3002","invalid_image_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u56fe\u7247\u6587\u4ef6\u3002","invalid_video_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u89c6\u9891\u6587\u4ef6\u3002","invalid_audio_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u97f3\u9891\u6587\u4ef6\u3002"},"alert":{"confirm_button_text":"\u786e\u5b9a","cancel_button_text":"\u53d6\u6d88","widget_remove_confirm":"Remove this widget?"},"datepicker":{"previousMonth":"\u4e0a\u4e00\u4e2a\u6708","nextMonth":"\u4e0b\u4e00\u4e2a\u6708","months":["\u4e00\u6708","\u4e8c\u6708","\u4e09\u6708","\u56db\u6708","\u4e94\u6708","\u516d\u6708","\u4e03\u6708","\u516b\u6708","\u4e5d\u6708","\u5341\u6708","\u5341\u4e00\u6708","\u5341\u4e8c\u6708"],"weekdays":["\u5468\u65e5","\u5468\u4e00","\u5468\u4e8c","\u5468\u4e09","\u5468\u56db","\u5468\u4e94","\u5468\u516d"],"weekdaysShort":["\u65e5","\u4e00","\u4e8c","\u4e09","\u56db","\u4e94","\u516d"]},"colorpicker":{"last_color":"Use previously selected color","aria_palette":"Color selection area","aria_hue":"Hue selection slider","aria_opacity":"Opacity selection slider"},"filter":{"group":{"all":"\u5168\u90e8"},"scopes":{"apply_button_text":"Apply","clear_button_text":"Clear"},"dates":{"all":"\u5168\u90e8","filter_button_text":"\u7b5b\u9009","reset_button_text":"\u91cd\u7f6e","date_placeholder":"\u65e5\u671f","after_placeholder":"After","before_placeholder":"Before"},"numbers":{"all":"all","filter_button_text":"Filter","reset_button_text":"Reset","min_placeholder":"Min","max_placeholder":"Max"}},"eventlog":{"show_stacktrace":"\u663e\u793a\u5806\u6808","hide_stacktrace":"\u9690\u85cf\u5806\u6808","tabs":{"formatted":"\u683c\u5f0f\u5316\u7684","raw":"\u539f\u59cb"},"editor":{"title":"\u6e90\u4ee3\u7801\u7f16\u8f91\u5668","description":"Your operating system should be configured to listen to one of these URL schemes.","openWith":"Open with","remember_choice":"\u8bb0\u4f4f\u9009\u62e9","open":"\u6253\u5f00","cancel":"\u53d6\u6d88"}}} | ||
| {"markdowneditor":{"formatting":"\u683c\u5f0f\u5316","quote":"\u5f15\u7528","code":"\u4ee3\u7801","header1":"\u6807\u9898 1","header2":"\u6807\u9898 2","header3":"\u6807\u9898 3","header4":"\u6807\u9898 4","header5":"\u6807\u9898 5","header6":"\u6807\u9898 6","bold":"\u7c97\u4f53","italic":"\u659c\u4f53","unorderedlist":"\u65e0\u5e8f\u5217\u8868","orderedlist":"\u6709\u5e8f\u5217\u8868","video":"\u89c6\u9891","image":"\u56fe\u7247","link":"\u94fe\u63a5","horizontalrule":"\u63d2\u5165\u5206\u5272\u7ebf","fullscreen":"\u5168\u5c4f","preview":"\u9884\u89c8"},"mediamanager":{"insert_link":"\u63d2\u5165\u94fe\u63a5","insert_image":"\u63d2\u5165\u56fe\u7247","insert_video":"\u63d2\u5165\u89c6\u9891","insert_audio":"\u63d2\u5165\u97f3\u9891","invalid_file_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u6587\u4ef6\u3002","invalid_file_single_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u6587\u4ef6\u3002","invalid_image_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u56fe\u7247\u6587\u4ef6\u3002","invalid_video_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u89c6\u9891\u6587\u4ef6\u3002","invalid_audio_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u97f3\u9891\u6587\u4ef6\u3002"},"alert":{"confirm_button_text":"\u786e\u5b9a","cancel_button_text":"\u53d6\u6d88","widget_remove_confirm":"Remove this widget?"},"popup":{"unsaved_changes":"The popup contains unsaved changes."},"datepicker":{"previousMonth":"\u4e0a\u4e00\u4e2a\u6708","nextMonth":"\u4e0b\u4e00\u4e2a\u6708","months":["\u4e00\u6708","\u4e8c\u6708","\u4e09\u6708","\u56db\u6708","\u4e94\u6708","\u516d\u6708","\u4e03\u6708","\u516b\u6708","\u4e5d\u6708","\u5341\u6708","\u5341\u4e00\u6708","\u5341\u4e8c\u6708"],"weekdays":["\u5468\u65e5","\u5468\u4e00","\u5468\u4e8c","\u5468\u4e09","\u5468\u56db","\u5468\u4e94","\u5468\u516d"],"weekdaysShort":["\u65e5","\u4e00","\u4e8c","\u4e09","\u56db","\u4e94","\u516d"]},"colorpicker":{"last_color":"Use previously selected color","aria_palette":"Color selection area","aria_hue":"Hue selection slider","aria_opacity":"Opacity selection slider"},"filter":{"group":{"all":"\u5168\u90e8"},"scopes":{"apply_button_text":"Apply","clear_button_text":"Clear"},"dates":{"all":"\u5168\u90e8","filter_button_text":"\u7b5b\u9009","reset_button_text":"\u91cd\u7f6e","date_placeholder":"\u65e5\u671f","after_placeholder":"After","before_placeholder":"Before"},"numbers":{"all":"all","filter_button_text":"Filter","reset_button_text":"Reset","min_placeholder":"Min","max_placeholder":"Max"}},"eventlog":{"show_stacktrace":"\u663e\u793a\u5806\u6808","hide_stacktrace":"\u9690\u85cf\u5806\u6808","tabs":{"formatted":"\u683c\u5f0f\u5316\u7684","raw":"\u539f\u59cb"},"editor":{"title":"\u6e90\u4ee3\u7801\u7f16\u8f91\u5668","description":"Your operating system should be configured to listen to one of these URL schemes.","openWith":"Open with","remember_choice":"\u8bb0\u4f4f\u9009\u62e9","open":"\u6253\u5f00","cancel":"\u53d6\u6d88"}}} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the Simplified Chinese numeric-filter translation.
This regenerated line changes filter.numbers.all from "全部" to "all", causing an unrelated English label in the Simplified Chinese UI. Restore the original translation while retaining popup.unsaved_changes.
Proposed fix
- "numbers":{"all":"all",
+ "numbers":{"all":"全部",📝 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.
| {"markdowneditor":{"formatting":"\u683c\u5f0f\u5316","quote":"\u5f15\u7528","code":"\u4ee3\u7801","header1":"\u6807\u9898 1","header2":"\u6807\u9898 2","header3":"\u6807\u9898 3","header4":"\u6807\u9898 4","header5":"\u6807\u9898 5","header6":"\u6807\u9898 6","bold":"\u7c97\u4f53","italic":"\u659c\u4f53","unorderedlist":"\u65e0\u5e8f\u5217\u8868","orderedlist":"\u6709\u5e8f\u5217\u8868","video":"\u89c6\u9891","image":"\u56fe\u7247","link":"\u94fe\u63a5","horizontalrule":"\u63d2\u5165\u5206\u5272\u7ebf","fullscreen":"\u5168\u5c4f","preview":"\u9884\u89c8"},"mediamanager":{"insert_link":"\u63d2\u5165\u94fe\u63a5","insert_image":"\u63d2\u5165\u56fe\u7247","insert_video":"\u63d2\u5165\u89c6\u9891","insert_audio":"\u63d2\u5165\u97f3\u9891","invalid_file_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u6587\u4ef6\u3002","invalid_file_single_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u6587\u4ef6\u3002","invalid_image_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u56fe\u7247\u6587\u4ef6\u3002","invalid_video_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u89c6\u9891\u6587\u4ef6\u3002","invalid_audio_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u97f3\u9891\u6587\u4ef6\u3002"},"alert":{"confirm_button_text":"\u786e\u5b9a","cancel_button_text":"\u53d6\u6d88","widget_remove_confirm":"Remove this widget?"},"popup":{"unsaved_changes":"The popup contains unsaved changes."},"datepicker":{"previousMonth":"\u4e0a\u4e00\u4e2a\u6708","nextMonth":"\u4e0b\u4e00\u4e2a\u6708","months":["\u4e00\u6708","\u4e8c\u6708","\u4e09\u6708","\u56db\u6708","\u4e94\u6708","\u516d\u6708","\u4e03\u6708","\u516b\u6708","\u4e5d\u6708","\u5341\u6708","\u5341\u4e00\u6708","\u5341\u4e8c\u6708"],"weekdays":["\u5468\u65e5","\u5468\u4e00","\u5468\u4e8c","\u5468\u4e09","\u5468\u56db","\u5468\u4e94","\u5468\u516d"],"weekdaysShort":["\u65e5","\u4e00","\u4e8c","\u4e09","\u56db","\u4e94","\u516d"]},"colorpicker":{"last_color":"Use previously selected color","aria_palette":"Color selection area","aria_hue":"Hue selection slider","aria_opacity":"Opacity selection slider"},"filter":{"group":{"all":"\u5168\u90e8"},"scopes":{"apply_button_text":"Apply","clear_button_text":"Clear"},"dates":{"all":"\u5168\u90e8","filter_button_text":"\u7b5b\u9009","reset_button_text":"\u91cd\u7f6e","date_placeholder":"\u65e5\u671f","after_placeholder":"After","before_placeholder":"Before"},"numbers":{"all":"all","filter_button_text":"Filter","reset_button_text":"Reset","min_placeholder":"Min","max_placeholder":"Max"}},"eventlog":{"show_stacktrace":"\u663e\u793a\u5806\u6808","hide_stacktrace":"\u9690\u85cf\u5806\u6808","tabs":{"formatted":"\u683c\u5f0f\u5316\u7684","raw":"\u539f\u59cb"},"editor":{"title":"\u6e90\u4ee3\u7801\u7f16\u8f91\u5668","description":"Your operating system should be configured to listen to one of these URL schemes.","openWith":"Open with","remember_choice":"\u8bb0\u4f4f\u9009\u62e9","open":"\u6253\u5f00","cancel":"\u53d6\u6d88"}}} | |
| {"markdowneditor":{"formatting":"\u683c\u5f0f\u5316","quote":"\u5f15\u7528","code":"\u4ee3\u7801","header1":"\u6807\u9898 1","header2":"\u6807\u9898 2","header3":"\u6807\u9898 3","header4":"\u6807\u9898 4","header5":"\u6807\u9898 5","header6":"\u6807\u9898 6","bold":"\u7c97\u4f53","italic":"\u659c\u4f53","unorderedlist":"\u65e0\u5e8f\u5217\u8868","orderedlist":"\u6709\u5e8f\u5217\u8868","video":"\u89c6\u9891","image":"\u56fe\u7247","link":"\u94fe\u63a5","horizontalrule":"\u63d2\u5165\u5206\u5272\u7ebf","fullscreen":"\u5168\u5c4f","preview":"\u9884\u89c8"},"mediamanager":{"insert_link":"\u63d2\u5165\u94fe\u63a5","insert_image":"\u63d2\u5165\u56fe\u7247","insert_video":"\u63d2\u5165\u89c6\u9891","insert_audio":"\u63d2\u5165\u97f3\u9891","invalid_file_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u6587\u4ef6\u3002","invalid_file_single_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u6587\u4ef6\u3002","invalid_image_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u56fe\u7247\u6587\u4ef6\u3002","invalid_video_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u89c6\u9891\u6587\u4ef6\u3002","invalid_audio_empty_insert":"\u8bf7\u9009\u62e9\u8981\u63d2\u5165\u7684\u97f3\u9891\u6587\u4ef6\u3002"},"alert":{"confirm_button_text":"\u786e\u5b9a","cancel_button_text":"\u53d6\u6d88","widget_remove_confirm":"Remove this widget?"},"popup":{"unsaved_changes":"The popup contains unsaved changes."},"datepicker":{"previousMonth":"\u4e0a\u4e00\u4e2a\u6708","nextMonth":"\u4e0b\u4e00\u4e2a\u6708","months":["\u4e00\u6708","\u4e8c\u6708","\u4e09\u6708","\u56db\u6708","\u4e94\u6708","\u516d\u6708","\u4e03\u6708","\u516b\u6708","\u4e5d\u6708","\u5341\u6708","\u5341\u4e00\u6708","\u5341\u4e8c\u6708"],"weekdays":["\u5468\u65e5","\u5468\u4e00","\u5468\u4e8c","\u5468\u4e09","\u5468\u56db","\u5468\u4e94","\u5468\u516d"],"weekdaysShort":["\u65e5","\u4e00","\u4e8c","\u4e09","\u56db","\u4e94","\u516d"]},"colorpicker":{"last_color":"Use previously selected color","aria_palette":"Color selection area","aria_hue":"Hue selection slider","aria_opacity":"Opacity selection slider"},"filter":{"group":{"all":"\u5168\u90e8"},"scopes":{"apply_button_text":"Apply","clear_button_text":"Clear"},"dates":{"all":"\u5168\u90e8","filter_button_text":"\u7b5b\u9009","reset_button_text":"\u91cd\u7f6e","date_placeholder":"\u65e5\u671f","after_placeholder":"After","before_placeholder":"Before"},"numbers":{"all":"\u5168\u90e8","filter_button_text":"Filter","reset_button_text":"Reset","min_placeholder":"Min","max_placeholder":"Max"}},"eventlog":{"show_stacktrace":"\u663e\u793a\u5806\u6808","hide_stacktrace":"\u9690\u85cf\u5806\u6808","tabs":{"formatted":"\u683c\u5f0f\u5316\u7684","raw":"\u539f\u59cb"},"editor":{"title":"\u6e90\u4ee3\u7801\u7f16\u8f91\u5668","description":"Your operating system should be configured to listen to one of these URL schemes.","openWith":"Open with","remember_choice":"\u8bb0\u4f4f\u9009\u62e9","open":"\u6253\u5f00","cancel":"\u53d6\u6d88"}}} |
🤖 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 `@modules/system/assets/js/lang/lang.zh-cn.js` at line 9, Restore
filter.numbers.all in the Simplified Chinese translation to the original Chinese
value “全部” instead of “all”. Preserve the existing popup.unsaved_changes entry
and all other regenerated translations unchanged.
- be: Insert Horizontal Rul -> Insert Horizontal Rule - nl: Volgende maan -> Volgende maand Both surfaced by the client lang recompile in this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- pl: stray leading quote in markdowneditor.unorderedlist - pt-br: Filtar -> Filtrar Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@AIC-BV what's with all the lang files changes? |
|
@mjauvin claude claims these are compiled automatically by running Then the AI rabbit started debugging these and found typos so I told claude to fix them as well |
4 participants
Add the possibility to define the popup size and its custom CSS classes.
You can now define these properties at the manage part of the config_relation.yaml:
See all popup sizes at https://wintercms.com/docs/v1.2/ui/controls/popup#data-attributes
sizealready existed, but was hardcoded and not dynamically filled in.cssClassis added.Summary by CodeRabbit
Release Notes