Merge "Clean up popup type settings related code"

src/actions.js

@@ -423,7 +423,7 @@ export function hideSettings() {
  * See docs/adr/0003-keep-enabled-state-only-in-preview-reducer.md for more
  * details.
  *
- * @param {Object} enabled
+ * @param {Object} enabled Mapping preview type identifiers to boolean flags
  * @return {Redux.Thunk}
  */
 export function saveSettings( enabled ) {

src/changeListeners/settings.js

@@ -1,5 +1,3 @@
-import { previewTypes } from '../preview/model';
-
 /**
  * Creates an instance of the settings change listener.
  *
@@ -28,11 +26,7 @@ export default function settings( boundActions, render ) {
 			}
 
 			// Update the UI settings with the current settings
-			settingsObj.setEnabled( previewTypes.TYPE_PAGE,
-				newState.preview.enabled[ previewTypes.TYPE_PAGE ] );
-			settingsObj.setEnabled( previewTypes.TYPE_REFERENCE,
-				newState.preview.enabled[ previewTypes.TYPE_REFERENCE ] );
-
+			settingsObj.setEnabled( newState.preview.enabled );
 			settingsObj.show();
 		} else if (
 			oldState.settings.shouldShow === true &&

src/changeListeners/syncUserSettings.js

@@ -46,9 +46,10 @@ export default function syncUserSettings( userSettings ) {
  * @return {*}
  */
 function get( state, path ) {
-	return path.split( '.' ).reduce( function ( element, key ) {
-		return element && element[ key ];
-	}, state );
+	return path.split( '.' ).reduce(
+		( element, key ) => element && element[ key ],
+		state
+	);
 }
 
 /**

src/reducers/settings.js

@@ -29,17 +29,15 @@ export default function settings( state, action ) {
 				showHelp: false
 			} );
 		case actionTypes.SETTINGS_CHANGE: {
-			const nothingChanged = Object.keys( action.newValue ).every( function ( type ) {
-					return action.oldValue[ type ] === action.newValue[ type ];
-				} ),
-				userOptedOut = Object.keys( action.newValue ).some( function ( type ) {
-					// Check if at least one setting has been deactivated that was active before
-					return action.oldValue[ type ] && !action.newValue[ type ];
-				} ),
-				anyDisabled = Object.keys( action.newValue ).some( function ( type ) {
-					// Warning, when the state is null the user can't re-enable this popup type!
-					return action.newValue[ type ] === false;
-				} );
+			const types = Object.keys( action.newValue ),
+				nothingChanged = types
+					.every( ( type ) => action.oldValue[ type ] === action.newValue[ type ] ),
+				// Check if at least one setting has been deactivated that was active before
+				userOptedOut = types
+					.some( ( type ) => action.oldValue[ type ] && !action.newValue[ type ] ),
+				// Warning, when the state is null the user can't re-enable this popup type!
+				anyDisabled = types
+					.some( ( type ) => action.newValue[ type ] === false );
 
 			if ( nothingChanged ) {
 				// If the setting is the same, just hide the dialogs
@@ -60,10 +58,10 @@ export default function settings( state, action ) {
 			} );
 		}
 		case actionTypes.BOOT: {
-			const anyDisabled = Object.keys( action.initiallyEnabled ).some( function ( type ) {
-				// Warning, when the state is null the user can't re-enable this popup type!
-				return action.initiallyEnabled[ type ] === false;
-			} );
+			// Warning, when the state is null the user can't re-enable this popup type!
+			const anyDisabled = Object.keys( action.initiallyEnabled )
+				.some( ( type ) => action.initiallyEnabled[ type ] === false );
+
 			return nextState( state, {
 				shouldShowFooterLink: action.user.isAnon && anyDisabled
 			} );

src/ui/settingsDialogRenderer.js

@@ -3,7 +3,6 @@
  */
 
 import { createSettingsDialog } from './settingsDialog';
-import { previewTypes } from '../preview/model';
 
 /**
  * Creates a render function that will create the settings dialog and return
@@ -40,16 +39,11 @@ export default function createSettingsDialogRenderer( config ) {
 			// Setup event bindings
 
 			$dialog.find( '.save' ).on( 'click', () => {
-				const $page = $dialog.find( '#mwe-popups-settings-' + previewTypes.TYPE_PAGE ),
-					$ref = $dialog.find( '#mwe-popups-settings-' + previewTypes.TYPE_REFERENCE );
-				boundActions.saveSettings( {
-					[ previewTypes.TYPE_PAGE ]: $page.is( ':checked' ),
-					[ previewTypes.TYPE_REFERENCE ]:
-						// TODO: Remove when not in Beta any more
-						config.get( 'wgPopupsReferencePreviewsBetaFeature' ) ?
-							null :
-							$ref.is( ':checked' )
+				const enabled = {};
+				$dialog.find( 'input' ).each( ( index, el ) => {
+					enabled[ el.value ] = $( el ).is( ':checked' );
 				} );
+				boundActions.saveSettings( enabled );
 			} );
 			$dialog.find( '.close, .okay' ).on( 'click', boundActions.hideSettings );
 		}
@@ -91,11 +85,13 @@ export default function createSettingsDialogRenderer( config ) {
 			/**
 			 * Update the form depending on the enabled flag
 			 *
-			 * @param {string} type
-			 * @param {boolean} enabled
+			 * @param {Object} enabled Mapping preview type identifiers to boolean flags
 			 */
-			setEnabled( type, enabled ) {
-				$dialog.find( '#mwe-popups-settings-' + type ).prop( 'checked', enabled );
+			setEnabled( enabled ) {
+				Object.keys( enabled ).forEach( ( type ) => {
+					$dialog.find( '#mwe-popups-settings-' + type )
+						.prop( 'checked', enabled[ type ] );
+				} );
 			}
 		};
 	};

tests/node-qunit/changeListeners/settings.test.js

@@ -85,11 +85,13 @@ QUnit.test( 'it should show settings and update the form when shouldShow becomes
 
 	assert.strictEqual( this.render.callCount, 1,
 		'The renderer should be called only the first time' );
-	assert.strictEqual( this.rendered.setEnabled.callCount, 4,
+	assert.strictEqual( this.rendered.setEnabled.callCount, 2,
 		'The rendered form should be up to date when shown' );
-	assert.strictEqual( this.rendered.setEnabled.firstCall.args[ 1 ], true,
+	assert.deepEqual( this.rendered.setEnabled.firstCall.args[ 0 ],
+		{ page: true, reference: true },
 		'Set enabled should be called with the current enabled state' );
-	assert.strictEqual( this.rendered.setEnabled.thirdCall.args[ 1 ], false,
+	assert.deepEqual( this.rendered.setEnabled.secondCall.args[ 0 ],
+		{ page: false, reference: true },
 		'Set enabled should be called with the current enabled state' );
 	assert.strictEqual( this.rendered.show.callCount, 2,
 		'The rendered object should be showed' );

tests/node-qunit/getPageviewTracker.test.js

@@ -1,7 +1,7 @@
 import getPageviewTracker, { getSendBeacon, limitByEncodedURILength } from '../../src/getPageviewTracker';
 
 QUnit.module( 'ext.popups#getPageviewTracker', {
-	beforeEach: function () {
+	beforeEach() {
 		this.makeBeaconUrl = this.sandbox.stub();
 		this.prepare = this.sandbox.stub();
 		this.trackerGetter = () => ( { makeBeaconUrl: this.makeBeaconUrl,

tests/node-qunit/ui/renderer.test.js

@@ -998,9 +998,7 @@ QUnit.test( '#layoutPreview - no thumbnail', ( assert ) => {
 	renderer.layoutPreview( preview, layout, classes, 200, 8, windowHeight );
 
 	assert.ok(
-		classes.every( function ( c ) {
-			return preview.el.hasClass( c );
-		} ),
+		classes.every( ( c ) => preview.el.hasClass( c ) ),
 		'Classes have been added.'
 	);
 	assert.strictEqual(
@@ -1035,9 +1033,7 @@ QUnit.test( '#layoutPreview - tall preview, flipped X, has thumbnail', function
 	renderer.layoutPreview( preview, layout, classes, 200, 8, windowHeight );
 
 	assert.ok(
-		classes.every( function ( c ) {
-			return preview.el.hasClass( c );
-		} ),
+		classes.every( ( c ) => preview.el.hasClass( c ) ),
 		'Classes have been added.'
 	);
 	assert.strictEqual(
@@ -1081,9 +1077,7 @@ QUnit.test( '#layoutPreview - portrait preview, flipped X, has thumbnail, small
 	renderer.layoutPreview( preview, layout, classes, 200, 8, windowHeight );
 
 	assert.ok(
-		classes.every( function ( c ) {
-			return preview.el.hasClass( c );
-		} ),
+		classes.every( ( c ) => preview.el.hasClass( c ) ),
 		'Classes have been added.'
 	);
 	assert.strictEqual(
@@ -1128,9 +1122,7 @@ QUnit.test( '#layoutPreview - portrait preview, flipped X, has thumbnail, big he
 	renderer.layoutPreview( preview, layout, classes, 200, 8, windowHeight );
 
 	assert.ok(
-		classes.every( function ( c ) {
-			return preview.el.hasClass( c );
-		} ),
+		classes.every( ( c ) => preview.el.hasClass( c ) ),
 		'Classes have been added.'
 	);
 	assert.strictEqual(
@@ -1171,9 +1163,7 @@ QUnit.test( '#layoutPreview - tall preview, has thumbnail, flipped Y', ( assert
 	renderer.layoutPreview( preview, layout, classes, 200, 8, windowHeight );
 
 	assert.ok(
-		classes.every( function ( c ) {
-			return preview.el.hasClass( c );
-		} ),
+		classes.every( ( c ) => preview.el.hasClass( c ) ),
 		'Classes have been added.'
 	);
 
@@ -1214,9 +1204,7 @@ QUnit.test( '#layoutPreview - tall preview, has thumbnail, flipped X and Y', fun
 	renderer.layoutPreview( preview, layout, classes, 200, 8, windowHeight );
 
 	assert.ok(
-		classes.every( function ( c ) {
-			return preview.el.hasClass( c );
-		} ),
+		classes.every( ( c ) => preview.el.hasClass( c ) ),
 		'Classes have been added.'
 	);
 	assert.strictEqual(
@@ -1252,9 +1240,7 @@ QUnit.test( '#layoutPreview - portrait preview, has thumbnail, flipped X and Y',
 	renderer.layoutPreview( preview, layout, classes, 200, 8, windowHeight );
 
 	assert.ok(
-		classes.every( function ( c ) {
-			return preview.el.hasClass( c );
-		} ),
+		classes.every( ( c ) => preview.el.hasClass( c ) ),
 		'Classes have been added.'
 	);
 	assert.strictEqual(

webpack.config.js

@@ -112,8 +112,8 @@ module.exports = ( env, argv ) => ( {
 		// Minified uncompressed size limits for chunks / assets and entrypoints. Keep these numbers
 		// up-to-date and rounded to the nearest 10th of a kibibyte so that code sizing costs are
 		// well understood. Related to bundlesize minified, gzipped compressed file size tests.
-		maxAssetSize: 44.5 * 1024,
-		maxEntrypointSize: 44.5 * 1024,
+		maxAssetSize: 44.1 * 1024,
+		maxEntrypointSize: 44.1 * 1024,
 
 		// The default filter excludes map files but we rename ours.
 		assetFilter: ( filename ) => !filename.endsWith( srcMapExt )