fix: Handle partial page updates #54
Merged
Conversation
This file contains 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
Reviewer's Guide by SourceryThis pull request addresses an issue where the editor would not properly re-initialize when parts of the page were updated. This was caused by the editor not being properly destroyed and re-initialized when the page was updated. The fix ensures that the editor is properly destroyed and re-initialized when the page is updated, and also ensures that the editor is not initialized twice when an inline editor is scrolled back into the viewport. Sequence diagram for editor re-initialization on page updatesequenceDiagram
participant Page
participant CMSEditor
participant Observer
participant EditorPlugin
Page->>CMSEditor: Page content updated
CMSEditor->>CMSEditor: Check if editor exists
alt Editor already initialized
CMSEditor-->>Page: Skip initialization
else Editor not initialized
CMSEditor->>Observer: Add element to observer
CMSEditor->>EditorPlugin: Create editor instance
CMSEditor->>CMSEditor: Store editor settings
end
Class diagram showing updated CMSEditor structureclassDiagram
class CMSEditor {
-_global_settings: Object
-_editor_settings: Object
-_generic_editors: Object
-_admin_selector: String
-_admin_add_row_selector: String
-_inline_admin_selector: String
+init(el)
+initAll()
+destroyAll()
+destroyRTE()
+destroyGenericEditor()
-_createRTE(el)
-_initInlineRichText(elements, url, cls)
-getSettings(el)
-_resetInlineEditors()
}
note for CMSEditor "Changed from array-based
to object-based storage
for better handling of
partial updates"
State diagram for editor initialization processstateDiagram-v2
[*] --> CheckEditor
CheckEditor --> AlreadyInitialized: Editor ID exists
CheckEditor --> Initialize: Editor ID not found
Initialize --> CreateRTE: TextPlugin/HTMLField
Initialize --> CreateGenericEditor: CharField
CreateRTE --> AddEventListeners
CreateGenericEditor --> AddEventListeners
AddEventListeners --> [*]
AlreadyInitialized --> [*]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
=======================================
Coverage 81.22% 81.22%
=======================================
Files 17 17
Lines 932 932
Branches 104 104
=======================================
Hits 757 757
Misses 132 132
Partials 43 43 ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @fsbraun - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
182
to
205
| if (wrapper) { | ||
| // Catch CMS single click event to highlight the plugin | ||
| // Catch CMS double click event if present, since double click is needed by Editor | ||
| this.observer.observe(wrapper); | ||
| if (this.CMS) { | ||
| // Remove django CMS core's double click event handler which opens an edit dialog | ||
| this.CMS.$(wrapper).off('dblclick.cms.plugin') | ||
| .on('dblclick.cms-editor', function (event) { | ||
| event.stopPropagation(); | ||
| }); | ||
| wrapper.addEventListener('focusin.cms-editor', () => { | ||
| this._highlightTextplugin(id); | ||
| }, true); | ||
| // Prevent tooltip on hover | ||
| this.CMS.$(wrapper).off('pointerover.cms.plugin pointerout.cms.plugin') | ||
| .on('pointerover.cms-editor', function (event) { | ||
| window.CMS.API.Tooltip.displayToggle(false, event.target, '', id); | ||
| event.stopPropagation(); | ||
| }); | ||
| if (!Array.from(this.observer.root?.children || []).includes(wrapper)) { | ||
| // Only add to the observer if not already observed (e.g., if the page only was update partially) | ||
| this.observer.observe(wrapper); | ||
| if (this.CMS) { | ||
| // Remove django CMS core's double click event handler which opens an edit dialog | ||
| this.CMS.$(wrapper).off('dblclick.cms.plugin') | ||
| .on('dblclick.cms-editor', function (event) { | ||
| event.stopPropagation(); | ||
| }); | ||
| wrapper.addEventListener('focusin', () => { | ||
| this._highlightTextplugin(id); | ||
| }, true); | ||
| // Prevent tooltip on hover | ||
| this.CMS.$(wrapper).off('pointerover.cms.plugin pointerout.cms.plugin') | ||
| .on('pointerover.cms-editor', function (event) { | ||
| window.CMS.API.Tooltip.displayToggle(false, event.target, '', id); | ||
| event.stopPropagation(); | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| if (wrapper) { | |
| // Catch CMS single click event to highlight the plugin | |
| // Catch CMS double click event if present, since double click is needed by Editor | |
| this.observer.observe(wrapper); | |
| if (this.CMS) { | |
| // Remove django CMS core's double click event handler which opens an edit dialog | |
| this.CMS.$(wrapper).off('dblclick.cms.plugin') | |
| .on('dblclick.cms-editor', function (event) { | |
| event.stopPropagation(); | |
| }); | |
| wrapper.addEventListener('focusin.cms-editor', () => { | |
| this._highlightTextplugin(id); | |
| }, true); | |
| // Prevent tooltip on hover | |
| this.CMS.$(wrapper).off('pointerover.cms.plugin pointerout.cms.plugin') | |
| .on('pointerover.cms-editor', function (event) { | |
| window.CMS.API.Tooltip.displayToggle(false, event.target, '', id); | |
| event.stopPropagation(); | |
| }); | |
| if (!Array.from(this.observer.root?.children || []).includes(wrapper)) { | |
| // Only add to the observer if not already observed (e.g., if the page only was update partially) | |
| this.observer.observe(wrapper); | |
| if (this.CMS) { | |
| // Remove django CMS core's double click event handler which opens an edit dialog | |
| this.CMS.$(wrapper).off('dblclick.cms.plugin') | |
| .on('dblclick.cms-editor', function (event) { | |
| event.stopPropagation(); | |
| }); | |
| wrapper.addEventListener('focusin', () => { | |
| this._highlightTextplugin(id); | |
| }, true); | |
| // Prevent tooltip on hover | |
| this.CMS.$(wrapper).off('pointerover.cms.plugin pointerout.cms.plugin') | |
| .on('pointerover.cms-editor', function (event) { | |
| window.CMS.API.Tooltip.displayToggle(false, event.target, '', id); | |
| event.stopPropagation(); | |
| }); | |
| } | |
| } | |
| } | |
| if (wrapper && !Array.from(this.observer.root?.children || []).includes(wrapper)) { | |
| this.observer.observe(wrapper); | |
| if (this.CMS) { | |
| // Remove django CMS core's double click event handler which opens an edit dialog | |
| this.CMS.$(wrapper).off('dblclick.cms.plugin') | |
| .on('dblclick.cms-editor', function (event) { | |
| event.stopPropagation(); | |
| }); | |
| wrapper.addEventListener('focusin', () => { | |
| this._highlightTextplugin(id); | |
| }, true); | |
| // Prevent tooltip on hover | |
| this.CMS.$(wrapper).off('pointerover.cms.plugin pointerout.cms.plugin') | |
| .on('pointerover.cms-editor', function (event) { | |
| window.CMS.API.Tooltip.displayToggle(false, event.target, '', id); | |
| event.stopPropagation(); | |
| }); | |
| } | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
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.
Summary by Sourcery
Update the editor to handle partial page updates, addressing issues with inline editing and toolbar interactions. Improve handling of global settings and ensure editor components are correctly initialized and destroyed.
Bug Fixes:
Enhancements: