Skip to content

Commit

Permalink
Improve the hide-show logic of ULS
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
santhoshtr committed Oct 6, 2015
1 parent 423b533 commit d4de09f
Showing 1 changed file with 6 additions and 8 deletions.
14 changes: 6 additions & 8 deletions src/jquery.uls.core.js
Expand Up @@ -177,9 +177,6 @@
this.initialized = true;
}

// hide any other visible ULS
$( '.uls-menu' ).hide();

this.$menu.show();
this.$menu.scrollIntoView();
this.shown = true;
Expand Down Expand Up @@ -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 ) {
Expand Down Expand Up @@ -353,10 +354,7 @@
}
},

click: function ( e ) {
e.stopPropagation();
e.preventDefault();

click: function () {
if ( this.shown ) {
this.hide();
} else {
Expand Down

0 comments on commit d4de09f

Please sign in to comment.