-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix: toolbar bug 3.10.rc1 #7223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -736,7 +736,7 @@ var Toolbar = new Class({ | |||
_refreshMarkup: function(newToolbar) { | |||
const switcher = this.ui.toolbarSwitcher.detach(); | |||
|
|||
$(this.ui.toolbar[0]).replaceWith(newToolbar[0]); | |||
$(this.ui.toolbar).html(newToolbar.children()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this fix, it would awesome if you add some comments about what was wrong and how does your fix resolves that? Easier to review and refer in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx! Goes back to the analysis I wrote in the issue comments:
There's been a change to cms.toolbar.js
. Also affected is line 739:
django-cms/cms/static/cms/js/modules/cms.toolbar.js
Lines 736 to 740 in 04e8433
_refreshMarkup: function(newToolbar) { | |
const switcher = this.ui.toolbarSwitcher.detach(); | |
$(this.ui.toolbar[0]).replaceWith(newToolbar[0]); | |
Hypothesis: The DOM is changed without updating the reference in this.ui.toolbar
leaving the latter pointing to something that is not displayed and hence has the width 0.
Also, this.ui.toolbar
here belongs to the Toolbar
class. Besides this shortcut for jQuery objects also a similar shortcut in the StructureBoard
class in cms.structureboard.js
needs to point to a display element of the DOM.
It seems that before the above commit the DOM was patched and not replaced leaving the pointers in tact.
Fix implemented for line 739 in cms.toolbar.js
:
this.ui.toolbar.html(newToolbar.children());
This leaves the DOM element unchanged, to which the two this.ui.toolbar
point. Only the child nodes are replaced and this.ui.toolbar
always points on a visible element and retains its width.
Fix confirmed 👍 Great work @fsbraun - thank you very much 🎉 |
* Fix: toolbar bug 3.10.rc1 (#7223) * build: bump django from 3.2.5 to 3.2.12 in /docs (#7226) Bumps [django](https://github.com/django/django) from 3.2.5 to 3.2.12. - [Release notes](https://github.com/django/django/releases) - [Commits](django/django@3.2.5...3.2.12) --- updated-dependencies: - dependency-name: django dependency-type: indirect * Update 01-install.rst fix for #7185 * TASK: Add missing backticks Co-authored-by: fsbraun <fsbraun@gmx.de> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Simon Krull <simondotunix@gmail.com>
Description
Fixes issue #7216
Related resources
Implements #7216 (comment)
Checklist
develop