8000 Fix: toolbar bug 3.10.rc1 by fsbraun · Pull Request #7223 · django-cms/django-cms · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 1 commit into from
Feb 9, 2022
Merged

Fix: toolbar bug 3.10.rc1 #7223

merged 1 commit into from
Feb 9, 2022

Conversation

fsbraun
Copy link
Member
@fsbraun fsbraun commented Feb 9, 2022

Description

Fixes issue #7216

Related resources

Implements #7216 (comment)

Checklist

  • I have opened this pull request against develop
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on Slack to find a “pr review buddy” who is going to review my pull request.

@@ -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());
Copy link
Member

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.

Copy link
Member Author
@fsbraun fsbraun Feb 9, 2022

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:

_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.

@marksweb
Copy link
Member
marksweb commented Feb 9, 2022

Fix confirmed 👍

Great work @fsbraun - thank you very much 🎉

@marksweb marksweb merged commit 25c7b69 into django-cms:develop Feb 9, 2022
marksweb added a commit to marksweb/django-cms that referenced this pull request Feb 10, 2022
vinitkumar added a commit that referenced this pull request Feb 10, 2022
Co-authored-by: Vinit Kumar <mail@vinitkumar.me>
@marksweb marksweb mentioned this pull request Feb 10, 2022
marksweb added a commit to marksweb/django-cms that referenced this pull request Feb 10, 2022
marksweb pushed a commit that referenced this pull request Feb 10, 2022
marksweb added a commit that referenced this pull request Feb 10, 2022
Co-authored-by: Vinit Kumar <mail@vinitkumar.me>
crydotsnake pushed a commit that referenced this pull request Feb 13, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0