Skip to content
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

Fix toolbar being updated only on the first navigation action #1028

Merged
merged 3 commits into from Oct 30, 2020

Conversation

fredvd
Copy link
Sponsor Member

@fredvd fredvd commented Oct 23, 2020

Main issue: this fixes the links in the plone toolbar becoming outdated when one navigates more than once in in the folder contents view.

But wait, there's more...

What still is very strange and I think unwanted is that the toolbar updating is done twice. And it's not only the toolbar, for every navigation click in folder_contents, 3 ajax calls are made twice to the backend for:

  • @@render-toolbar (toolbar itself)
  • @@fc-contextInfo (actions in the toolbar)
  • @@getVocabulary (folder_contents main table)

Double event handlers?

When I inspect the registered event handlers on the body element in the Firefox devtools, the toolbar structure-changed event is present twice , once from jquery and once from DOM2|bubbling, but I have no clue if this causes the double refresh of the toolbar.
folder_contents first load events on body

Also: another click event handler in the Toolbar component is not properly unregistered before it's reregistered and pile up on the body tag when navigating :

$('body').on('click', function(event) {

Changing this line 247 to::

   $('body').off('click').on('click', function(event) {

Solves the click events not being cleaned up, but this is rather new for me, I'm already happy I kind of found the toolbar update isssue. :-) If somebody more knowledgable could confirm this is the case?

Copy link
Sponsor Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good change, and it works for me. I would like another review though.

I initially did not see a difference. I use current coredev 5.2 Python 3.8, with your mockup branch checked out. Fresh Plone Site. Set development mode On in resource registries, save it. That should be enough I think.
But after ./bin/plone-compile-resources, and turning development mode Off again, it works.

Scenario:

  • Create Fresh Plone Site.
  • Create private Folder.
  • Go to folder contents.
  • In folder contents go to the site root. You see the toolbar change, which worked already before, since this is the first click.
  • In the same folder contents go to the private folder. You see the toolbar change again. Most visibly: the review status changes. This is the part that did not work before.

@mauritsvanrees
Copy link
Sponsor Member

Had contact with Fred. After putting resource registries in development mode, I have to develop the plone-editor-tools and then it works.

@fredvd fredvd requested a review from agitator October 26, 2020 20:40
@@ -244,7 +244,7 @@ define([
}
});

$('body').on('click', function(event) {
$('body').off('click').on('click', function(event) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would unregister all events handlers bound to the body on "click" event. Modal dialogs closing on a click outside the dialogue could be a case.
To prevent this, you could factor out the inline callback into a normal function which you pass also the off method as second argument: https://api.jquery.com/off/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - I've got something wrong: You'd have to pass the callback as a third argument.
Instead do this:

Suggested change
$('body').off('click').on('click', function(event) {
$('body').off('click', 'FRED-PLEASE-FIND-A-NAME').on('click', 'FRED-PLEASE-FIND-A-NAME', function(event) {

please replace 'FRED-PLEASE-FIND-A-NAME' with a suitable name - quickly looking at the code I don't know what it does.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thet thanks! I've updated the .off().on() calls. To prevent removing all clicks you can add a .namespace value to the event name itself. I have changed the generic one with 'click.closetoolbarsubmenus'. It closes opened submenu's when an editor clicks outside the toolbar in the main content.

There are a few other places where we don't use namespaced but maybe should, I'll propose changed to thosein a separate pull request. First get this one fixed & released.

@thet
Copy link
Member

thet commented Oct 28, 2020

@fredvd I does look right except for the one comment but I'm currently short on time to reproduce the failure fully.
I can confirm the doubled requests, which shouldn't happen.
After fixing the issue I mentioned, go ahead and merge this.

…lick events when reregistering after a pattern update.
@fredvd fredvd merged commit 1cf6b04 into 3.x Oct 30, 2020
@mauritsvanrees
Copy link
Sponsor Member

Tested (in the compiled version from plone/plone.staticresources#105) and it works.

Should be done on master as well.

thet added a commit that referenced this pull request Feb 19, 2021
Do only remove the correct event listener on ``context-info-loaded`` before adding a new one.
Fixes a problem where the current path was not updated for the upload popup when changing paths.
Fixes: #1016
Refs: #1028, #1030, #1039
thet added a commit that referenced this pull request Feb 19, 2021
Do only remove the correct event listener on ``context-info-loaded`` before adding a new one.
Fixes a problem where the current path was not updated for the upload popup when changing paths.
Fixes: #1016
Refs: #1028, #1030, #1039
@fredvd fredvd deleted the folder_contents_nav_toolbar_update branch February 19, 2021 20:16
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.

None yet

3 participants