Conversation
There was a problem hiding this comment.
Pull request overview
Adds “save draft” support to MagicPreview so users can persist unsaved resource form state in cache and later restore/discard it, including UI affordances (banner + action buttons) and optional TTL via system setting.
Changes:
- Persist resource form payload as a per-resource/per-user draft in cache (optional TTL) when saving a draft.
- Add restore/discard draft processors (MODX 3 + MODX 2 variants) that restore via MODX’s reload registry.
- Add manager UI updates (banner, buttons, tooltips, icons) plus new lexicon strings and settings.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| core/components/magicpreview/processors/resource/restore-draft.class.php | MODX 3 restore draft processor writing to reload registry |
| core/components/magicpreview/processors/resource/restore-draft-v2.class.php | MODX 2 restore draft processor equivalent |
| core/components/magicpreview/processors/resource/discard-draft.class.php | MODX 3 discard draft processor |
| core/components/magicpreview/processors/resource/discard-draft-v2.class.php | MODX 2 discard draft processor |
| core/components/magicpreview/processors/resource/PreviewTrait.php | Adds draft persistence to existing preview submission flow |
| core/components/magicpreview/processors/resource/DraftTrait.php | Shared cache get/delete helpers for draft processors |
| core/components/magicpreview/lexicon/en/default.inc.php | Adds draft + tooltip + icon setting lexicon strings |
| core/components/magicpreview/lexicon/de/default.inc.php | Adds German draft + tooltip + icon setting lexicon strings |
| core/components/magicpreview/lexicon/da/default.inc.php | Adds Danish draft + tooltip + icon setting lexicon strings |
| core/components/magicpreview/elements/plugins/magicpreview.plugin.php | Injects draft presence info + icon HTML + lexicon into JS config |
| assets/components/magicpreview/js/preview.js | Adds save/restore/discard orchestration + banner + action bar button injection/tooltips |
| assets/components/magicpreview/js/panel.js | Adds Save Draft button to preview panel toolbar |
| assets/components/magicpreview/css/mgr.css | Styles draft banner + merged action bar buttons + panel save-draft button |
| assets/components/magicpreview/connector.php | Maps actions to MODX 2 processor variants when needed |
| _build/data/settings.php | Adds new system settings (draft TTL + icons) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Push the fixed-position action buttons below the banner */ | ||
| body:has(.mmmp-draft-banner) #modx-action-buttons { | ||
| top: 48px; | ||
| } | ||
|
|
There was a problem hiding this comment.
The CSS here relies on the :has() selector to detect when the draft banner exists and push #modx-action-buttons down. :has() is not supported in some browsers/environments, so these rules will be ignored and the banner may overlap the fixed action bar. Consider adding/removing a body class in JS when the banner is rendered (e.g. document.body.classList.add('mmmp-has-draft-banner')) and target that class instead of :has().
| /* Push the fixed-position action buttons below the banner */ | |
| body:has(.mmmp-draft-banner) #modx-action-buttons { | |
| top: 48px; | |
| } | |
| /* Push the fixed-position action buttons below the banner */ | |
| body.mmmp-has-draft-banner #modx-action-buttons, | |
| body:has(.mmmp-draft-banner) #modx-action-buttons { | |
| top: 48px; | |
| } | |
| body.magicpreview_modx2.mmmp-has-draft-banner #modx-action-buttons, |
| #modx-action-buttons .x-toolbar-cell:has(> #modx-abtn-real-preview) { | ||
| padding-right: 0; | ||
| } | ||
|
|
||
| #modx-action-buttons .x-toolbar-cell:has(> #modx-abtn-save-draft) { | ||
| padding-left: 0; | ||
| padding-right: 0; | ||
| } | ||
|
|
||
| #modx-action-buttons .x-toolbar-cell:has(> #modx-abtn-preview) { | ||
| padding-left: 0; | ||
| } |
There was a problem hiding this comment.
These selectors use :has() to remove toolbar-cell padding between specific buttons. If :has() is unsupported, the merged button-group styling won't apply and spacing/borders will look broken. Using explicit classes on the buttons/cells (added by JS after render) or targeting adjacent siblings would be more compatible.
| text: config().iconSaveDraft, | ||
| id: 'modx-abtn-save-draft', | ||
| handler: function() { saveDraft(); }, | ||
| scope: this |
There was a problem hiding this comment.
The Save Draft action-bar button is rendered as icon-only HTML via text: config().iconSaveDraft, but no accessible label is provided on the ExtJS button itself. Tooltips don’t reliably cover keyboard/screen reader use. Consider setting the button's tooltip and/or ariaLabel/text (with CSS hiding) so the control is accessible when icons are used.
| scope: this | |
| scope: this, | |
| tooltip: lexicon('save_draft'), | |
| ariaLabel: lexicon('save_draft') |
| ]); | ||
| if (!empty($draft) && is_array($draft) && !empty($draft['data'])) { | ||
| $jsConfig['hasDraft'] = true; | ||
| $jsConfig['draftSavedAt'] = date('Y-m-d H:i', (int) $draft['saved_at']); |
There was a problem hiding this comment.
$draft['saved_at'] is accessed without checking it exists. If drafts were created before this field was introduced (or cache contents are otherwise incomplete), this will emit a PHP notice and date() will fall back to the Unix epoch. Consider guarding with isset($draft['saved_at']) (and/or defaulting to time()), or ensuring older drafts are migrated/ignored safely.
| $jsConfig['draftSavedAt'] = date('Y-m-d H:i', (int) $draft['saved_at']); | |
| $savedAt = isset($draft['saved_at']) ? (int) $draft['saved_at'] : time(); | |
| $jsConfig['draftSavedAt'] = date('Y-m-d H:i', $savedAt); |
| $draftTtl = (int) $this->modx->getOption('magicpreview.draft_ttl', null, 0); | ||
| $draftKey = $this->object->get('id') . '/' . $this->modx->user->get('id'); | ||
|
|
There was a problem hiding this comment.
The draft cache key format (<resourceId>/<userId>) is now duplicated across multiple locations (here, DraftTrait, and the plugin). To avoid future drift, consider centralizing key generation (e.g., a shared helper/trait used by PreviewTrait too, or a service method/constant) and reuse it everywhere drafts are read/written.
Users can now save drafts for later. Drafts are saved in cache and there's a system setting for an optional TTL.
When a user returns to a resource with a draft saved by their user, they'll see a banner at the top displayed and they can choose to either restore it or discard it.