-
Notifications
You must be signed in to change notification settings - Fork 356
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 for Service Catalogs: Dialogs are hanging and keeps buffering #1197
Conversation
@@ -1,16 +1,20 @@ | |||
/* global miqInitSelectPicker miqSelectPickerEvent miqSparkle miqSparkleOn */ | |||
|
|||
var dialogFieldRefresh = { | |||
unbindAllPreviousListeners: function() { | |||
$(document).off('autoRefresh'); |
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.
@eclarizio is 'autoRefresh' our trigger event name - like the old 'message' that was pulled out? Or is it specific to JS?
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.
@syncrou Correct, instead of just a generic 'message' (which is how the postMessage
call was working), I just made it so the trigger event name is 'autoRefresh'. We could even be more specific if you wanted and make it something like 'dialog::autoRefresh' or whatever if you have a better idea or a clearer name, but this is just how jQuery does it and allows us to use the .off
method to unbind all the current listeners.
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 dialog::autoRefresh - or dialogAutoRefresh - just something to make it obvious it's our message. Thanks a lot for explanation!
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.
@eclarizio - Only request is to rename autoRefresh to make it a little more obvious we own that message.
Checked commits eclarizio/manageiq-ui-classic@071e3cd~...21f5220 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@dclarizio Holiday in Brno today. Is there someone that can review/merge this? Looks like @eclarizio addressed comments already. @miq-bot add_label blocker |
Fix for Service Catalogs: Dialogs are hanging and keeps buffering (cherry picked from commit 27a33c9) https://bugzilla.redhat.com/show_bug.cgi?id=1447088
Fine backport details:
|
Euwe backport (to manageiq repo) details:
|
This problem arose from having multiple dialogs with auto-refreshable fields, and then when cancelling an order of one, attempting to order a different one. Since it is not a full page refresh when cancelling, the javascript that is listening for the messages stays intact and attempts to auto-refresh a field that does not exist.
This fix will replace the old
postMessage
way of triggering auto refreshes for dialog fields with jQuery bindings and toggles. This then allows us to unbind all the previous listeners. As a plus, it is now easier to test thelistenForAutoRefreshMessages
function and I've included that as well.https://bugzilla.redhat.com/show_bug.cgi?id=1442811
@miq-bot add_label euwe/yes, fine/yes, bug
/cc @gmcculloug
@himdel Please review or tag someone else that can review. Thanks!