From 3af403fee59ecc4c9991f5f142ceead8c7c3007f Mon Sep 17 00:00:00 2001 From: Santhosh Thottingal Date: Tue, 27 Aug 2013 11:56:58 +0530 Subject: [PATCH] Make the cancel and apply button applicable for all modules If a user does changes in module A, does not save or cancel, goes to module B, does some changes, moves to other modules, and finally presses Apply, all changes should get saved. Similarly, if a user cancels, all changes should get cancelled. This required moving the cancel and apply button outside of modules and managed by the language settings framework. Modules get mw.uls.settings.apply or mw.uls.settings.cancel triggers to do whatever they want to do on apply or save. Includes some refactoring related to this. Bug: 53256 Change-Id: I7d773d33a980a78604b36e39bf96a5686870124e --- resources/css/ext.uls.languagesettings.css | 2 +- resources/js/ext.uls.displaysettings.js | 31 +++----------- resources/js/ext.uls.inputsettings.js | 36 ++++------------- resources/js/ext.uls.languagesettings.js | 40 ++++++++++++++----- .../features/step_definitions/panel_steps.rb | 8 ++-- .../uls_cog_sidebar_user_steps.rb | 6 +-- .../support/modules/interlanguage_module.rb | 2 - .../features/support/pages/panel_page.rb | 3 +- tests/browser/features/uls_triggers.feature | 3 -- 9 files changed, 51 insertions(+), 80 deletions(-) diff --git a/resources/css/ext.uls.languagesettings.css b/resources/css/ext.uls.languagesettings.css index 7c5e61f5..b7a1b94c 100644 --- a/resources/css/ext.uls.languagesettings.css +++ b/resources/css/ext.uls.languagesettings.css @@ -92,7 +92,7 @@ background-color: #F5F5F5; } -#languagesettings-settings-panel .language-settings-buttons { +.language-settings-buttons { border-top: 1px solid #F0F0F0; margin-top: 25px; padding: 15px; diff --git a/resources/js/ext.uls.displaysettings.js b/resources/js/ext.uls.displaysettings.js index 3b7e6583..cfcbe45e 100644 --- a/resources/js/ext.uls.displaysettings.js +++ b/resources/js/ext.uls.displaysettings.js @@ -78,19 +78,9 @@ + '' // End font selectors - + '' // End font settings section + + ''; // End font settings section - // Separator - + '
' - // Apply and Cancel buttons - + '
' - + '
' - + '' - + '' - + '
' - + '
' - + ''; function DisplaySettings( $parent ) { this.name = $.i18n( 'ext-uls-display-settings-title-short' ); @@ -452,8 +442,7 @@ * i18n this settings panel */ i18n: function () { - this.$template.i18n(); - + this.$parent.i18n(); this.$template.find( '#ui-font-selector-label strong' ) .text( $.i18n( 'ext-uls-webfonts-select-for', $.uls.data.getAutonym( this.uiLanguage ) ) ); this.$template.find( '#content-font-selector-label strong' ) @@ -485,11 +474,11 @@ */ markDirty: function () { this.dirty = true; - this.$template.find( '#uls-displaysettings-apply' ).removeAttr( 'disabled' ); + this.$parent.$window.find( 'button.uls-settings-apply' ).removeAttr( 'disabled' ); }, disableApplyButton: function () { - this.$template.find( '#uls-displaysettings-apply' ).prop( 'disabled', true ); + this.$parent.$window.find( 'button.uls-settings-apply' ).prop( 'disabled', true ); }, /** @@ -503,15 +492,6 @@ // TODO all these repeated selectors can be placed in object constructor. - displaySettings.$template.find( '#uls-displaysettings-apply' ).on( 'click', function () { - displaySettings.apply(); - } ); - - displaySettings.$template.find( 'button.uls-display-settings-cancel' ).on( 'click', function () { - displaySettings.cancel(); - displaySettings.close(); - } ); - $uiFontSelector.on( 'change', function () { displaySettings.markDirty(); mw.webfonts.preferences.setFont( displaySettings.uiLanguage, @@ -551,7 +531,6 @@ } ); - mw.hook( 'mw.uls.settings.cancel' ).add( $.proxy( this.cancel, this ) ); }, /** @@ -624,6 +603,7 @@ */ cancel: function () { if ( !this.dirty ) { + this.close(); return; } // Reload preferences @@ -637,6 +617,7 @@ // Restore content and UI language this.uiLanguage = this.getUILanguage(); this.contentLanguage = this.getContentLanguage(); + this.close(); } }; diff --git a/resources/js/ext.uls.inputsettings.js b/resources/js/ext.uls.inputsettings.js index 81bafbc8..c5f8808f 100644 --- a/resources/js/ext.uls.inputsettings.js +++ b/resources/js/ext.uls.inputsettings.js @@ -53,18 +53,6 @@ + '
' + '' + '
' - + '' - - // Separator - + '
' - - // Apply and Cancel buttons - + '
' - + '
' - + '' - + '' - + '
' - + '
' + ''; function InputSettings( $parent ) { @@ -76,6 +64,8 @@ this.$imes = null; this.$parent = $parent; this.savedRegistry = $.extend( true, {}, $.ime.preferences.registry ); + // ime system is lazy loaded, make sure it is initialized + mw.ime.init(); } InputSettings.prototype = { @@ -92,8 +82,6 @@ this.$imes = $( 'body' ).data( 'ime' ); this.$parent.$settingsPanel.append( this.$template ); $enabledOnly = this.$template.find( '.enabled-only' ); - // ime system is lazy loaded, make sure it is initialized - mw.ime.init(); if ( $.ime.preferences.isEnabled() ) { $enabledOnly.removeClass( 'hide' ); } else { @@ -103,8 +91,7 @@ this.prepareLanguages(); this.prepareToggleButton(); - this.$template.i18n(); - this.disableApplyButton(); + this.$parent.i18n(); $( 'body' ).data( 'webfonts' ).refresh(); this.listen(); }, @@ -115,11 +102,11 @@ */ markDirty: function () { this.dirty = true; - this.$template.find( 'button.uls-input-settings-apply' ).prop( 'disabled', false ); + this.$parent.$window.find( 'button.uls-settings-apply' ).prop( 'disabled', false ); }, disableApplyButton: function () { - this.$template.find( 'button.uls-input-settings-apply' ).prop( 'disabled', true ); + this.$parent.$window.find( 'button.uls-settings-apply' ).prop( 'disabled', true ); }, prepareInputmethods: function ( language ) { @@ -446,16 +433,6 @@ $imeListContainer = this.$template.find( '.uls-input-settings-inputmethods-list' ); - // Apply and close buttons - inputSettings.$template.find( 'button.uls-input-settings-apply' ).on( 'click', function () { - inputSettings.apply(); - } ); - - inputSettings.$template.find( 'button.uls-input-settings-cancel' ).on( 'click', function () { - inputSettings.cancel(); - inputSettings.close(); - } ); - $imeListContainer.on( 'change', 'input:radio[name=ime]:checked', function () { inputSettings.markDirty(); $.ime.preferences.setIM( $( this ).val() ); @@ -472,7 +449,6 @@ } } ); - mw.hook( 'mw.uls.settings.cancel' ).add( $.proxy( this.cancel, this ) ); }, /** @@ -564,6 +540,7 @@ */ cancel: function () { if ( !this.dirty ) { + this.close(); return; } // Reload preferences @@ -576,6 +553,7 @@ } else { mw.ime.disable(); } + this.close(); } }; diff --git a/resources/js/ext.uls.languagesettings.js b/resources/js/ext.uls.languagesettings.js index 02bfafb2..6e408154 100644 --- a/resources/js/ext.uls.languagesettings.js +++ b/resources/js/ext.uls.languagesettings.js @@ -20,7 +20,7 @@ ( function ( $, mw ) { 'use strict'; - var closeRow, settingsMenu, settingsPanel, windowTemplate, panelsRow; + var closeRow, settingsMenu, settingsPanel, windowTemplate, panelsRow, buttonsRow; closeRow = '
' + '
' + @@ -32,9 +32,19 @@ '
'; settingsPanel = '
' + '
'; + buttonsRow = '
' + + // Apply and Cancel buttons + '
' + + '
' + + '' + + '' + + '
' + + '
' + + ''; panelsRow = '
' + settingsMenu + settingsPanel + + buttonsRow + '
'; windowTemplate = '
' + closeRow + @@ -66,9 +76,11 @@ // Register all event listeners to the ULS language settings here. listen: function () { this.$element.on( 'click', $.proxy( this.click, this ) ); - this.$window.find( '#languagesettings-close' ) - .on( 'click', $.proxy( this.close, this ) ); + this.$window.find( '#languagesettings-close, button.uls-settings-cancel' ) + .on( 'click', $.proxy( mw.hook( 'mw.uls.settings.cancel' ).fire, this ) ); + this.$window.find( 'button.uls-settings-apply' ) + .on( 'click', $.proxy( mw.hook( 'mw.uls.settings.apply' ).fire, this ) ); // Hide the window when clicked outside $( 'html' ).click( $.proxy( this.hide, this ) ); @@ -95,17 +107,18 @@ } // Call render function on the current setting module. - languageSettings.renderModule( moduleName, defaultModule === moduleName ); + languageSettings.initModule( moduleName, defaultModule === moduleName ); } } ); }, /** + * Initialize the module. * Render the link and settings area for a language setting module. - * @param moduleName String Name of the setting module - * @param active boolean Make this module active and show by default + * @param {string} moduleName Name of the setting module + * @param {boolean} active boolean Make this module active and show by default */ - renderModule: function ( moduleName, active ) { + initModule: function ( moduleName, active ) { var $settingsTitle, $settingsText, $settingsLink, languageSettings = this, module = new $.fn.languagesettings.modules[moduleName]( languageSettings ), @@ -132,7 +145,7 @@ var $this = $( this ); $this.data( 'module' ).render(); - // re-position the window and scroll in to view if required. + // Re-position the window and scroll in to view if required. languageSettings.position(); $settingsMenuItems.find( '.menu-section' ).removeClass( 'active' ); $this.addClass( 'active' ); @@ -143,6 +156,10 @@ $settingsLink.addClass( 'active' ); } this.modules[moduleName] = module; + + // Register cancel and apply hooks + mw.hook( 'mw.uls.settings.cancel' ).add( $.proxy( module.cancel, module ) ); + mw.hook( 'mw.uls.settings.apply' ).add( $.proxy( module.apply, module ) ); }, position: function () { @@ -160,6 +177,10 @@ this.$window.scrollIntoView(); }, + i18n: function () { + this.$window.i18n(); + }, + show: function () { if ( !this.initialized ) { this.render(); @@ -167,7 +188,7 @@ } // close model windows close, if they hide on page click $( 'html' ).click(); - this.$window.i18n(); + this.i18n(); this.shown = true; this.$window.show(); @@ -217,7 +238,6 @@ this.options.onClose(); } - mw.hook( 'mw.uls.settings.cancel' ).fire(); }, click: function ( e ) { diff --git a/tests/browser/features/step_definitions/panel_steps.rb b/tests/browser/features/step_definitions/panel_steps.rb index 3caf222d..6737682b 100644 --- a/tests/browser/features/step_definitions/panel_steps.rb +++ b/tests/browser/features/step_definitions/panel_steps.rb @@ -68,10 +68,10 @@ Then(/^the selected (.*?) font must be "(.*?)"$/) do |type, font| end When(/^I apply the changes$/) do - on(ULSPage).panel_button_display_apply_element.click - wait = Selenium::WebDriver::Wait.new(:timeout => 3) - panel = @browser.driver.find_element(:id => 'language-settings-dialog') - wait.until { !panel.displayed? } + on(ULSPage).panel_button_apply_element.click + # Leave a little time for the settings to be saved. The settings window closes + # immediately, so it is not enough to wait for it to disappear. + sleep 2 end Then(/^the (.*) font must be changed to the "(.*?)" font$/) do |type, font| diff --git a/tests/browser/features/step_definitions/uls_cog_sidebar_user_steps.rb b/tests/browser/features/step_definitions/uls_cog_sidebar_user_steps.rb index 5b6d3605..77d525dc 100644 --- a/tests/browser/features/step_definitions/uls_cog_sidebar_user_steps.rb +++ b/tests/browser/features/step_definitions/uls_cog_sidebar_user_steps.rb @@ -27,12 +27,8 @@ Given(/^I navigate to the Language Settings panel$/) do step 'I see the logged in language settings panel' end -When(/^I click Apply Settings$/) do - on(InterlanguagePage).apply_settings_element.click -end - When(/^I click Cancel$/) do - on(InterlanguagePage).cancel_element.click + on(ULSPage).panel_button_cancel_element.click end When(/^I click on the link to select Malayalam$/) do diff --git a/tests/browser/features/support/modules/interlanguage_module.rb b/tests/browser/features/support/modules/interlanguage_module.rb index cd9dc4e1..275f03ca 100644 --- a/tests/browser/features/support/modules/interlanguage_module.rb +++ b/tests/browser/features/support/modules/interlanguage_module.rb @@ -4,10 +4,8 @@ module InterlanguagePageModule include PageObject a(:add_links, id: 'wbc-linkToItem-link') - span(:apply_settings, class: 'uls-settings-trigger') a(:back_to_display, text: 'Back to display settings') a(:back_to_input, text: 'Back to input settings') - button(:cancel, class: 'button uls-display-settings-cancel') span(:cog, class: 'uls-settings-trigger') button(:ellipsis_button, class: 'uls-more-languages button') a(:english_link, text: 'English') diff --git a/tests/browser/features/support/pages/panel_page.rb b/tests/browser/features/support/pages/panel_page.rb index fb159a46..d937b32e 100644 --- a/tests/browser/features/support/pages/panel_page.rb +++ b/tests/browser/features/support/pages/panel_page.rb @@ -10,7 +10,8 @@ class ULSPage button(:panel_language, id: 'uls-display-settings-language-tab') span(:panel_button_close, id: 'languagesettings-close') - button(:panel_button_display_apply, id: 'uls-displaysettings-apply') + button(:panel_button_apply, class: 'uls-settings-apply') + button(:panel_button_cancel, class: 'uls-settings-cancel') button(:panel_disable_input_methods, class: 'uls-input-toggle-button') button(:panel_enable_input_methods, class: 'uls-input-toggle-button') diff --git a/tests/browser/features/uls_triggers.feature b/tests/browser/features/uls_triggers.feature index 9dbb2417..319c803d 100644 --- a/tests/browser/features/uls_triggers.feature +++ b/tests/browser/features/uls_triggers.feature @@ -35,6 +35,3 @@ Feature: ULS trigger in personal toolbar | user status | close method | | logged in | I click X | | logged out | I click Cancel | - # It is weird that this works, since the button is disabled until changes - # have been made - | logged out | I click Apply Settings |