From d4de09ffb94dd9682c2b87a1cb2e910780acae27 Mon Sep 17 00:00:00 2001 From: Santhosh Thottingal Date: Wed, 30 Sep 2015 10:54:04 +0530 Subject: [PATCH] Improve the hide-show logic of ULS * The events are suppressed on click of ULS trigger, it is an anti pattern. It can cause other overlay dialogs, if any, to stay with ULS and causes UI glitch. This patch just propagates the events and not eats up. * The show method was hiding all other ULS dialoges open using a global $('.uls-menu').hide(). This is again not a good pattern. A plugin instance should not interfere with other instance's state. More over, calling jQuery hide() method on menu instead of plugin's hide method leaves the other plugin instance in a corrupted state. The plugin hide method does more things than just hiding the menu. It has a 'shown' book keeping property to update. This kind of corrupted state was causing bugs like https://phabricator.wikimedia.org/T114123 * While avoiding the above two antipatterns, the way ULS was hidden when clicked on any 'other' part of body was improved. It now uses event.target to correctly handle the 'click-outside-hide' logic All these above changes does not change any existing UX. Change-Id: I40b355115cbda54a68e8d58d3750fb9f1c3b6920 --- src/jquery.uls.core.js | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/jquery.uls.core.js b/src/jquery.uls.core.js index 201d384..529b47a 100644 --- a/src/jquery.uls.core.js +++ b/src/jquery.uls.core.js @@ -177,9 +177,6 @@ this.initialized = true; } - // hide any other visible ULS - $( '.uls-menu' ).hide(); - this.$menu.show(); this.$menu.scrollIntoView(); this.shown = true; @@ -321,7 +318,11 @@ /** * On cancel handler for the uls menu */ - cancel: function () { + cancel: function ( e ) { + if ( e && this.$element.is( e.target ) ) { + return; + } + this.hide(); if ( this.options.onCancel ) { @@ -353,10 +354,7 @@ } }, - click: function ( e ) { - e.stopPropagation(); - e.preventDefault(); - + click: function () { if ( this.shown ) { this.hide(); } else {