jenkins-bot has submitted this change and it was merged.
Change subject: Don't always render after the API request resolves
......................................................................
Don't always render after the API request resolves
If the user abandons the link after the API request delay (500 ms) but
before the it resolves (~10e3 ms), then the preview shouldn't be
rendered.
Changes:
* actions: Include the link in the FETCH_START, FETCH_FAILED, and
FETCH_END actions.
* reducers: If the active link has changed, then FETCH_END is a NOOP.
Supporting changes:
* reducers: Signal that a preview should be rendered and shown with
preview.shouldShow.
Change-Id: I3dd1c0c566ec63de515174c14845d7927583ce93
---
M resources/ext.popups/actions.js
M resources/ext.popups/reducers.js
M resources/ext.popups/renderChangeListener.js
M tests/qunit/ext.popups/actions.test.js
M tests/qunit/ext.popups/reducers.test.js
5 files changed, 111 insertions(+), 30 deletions(-)
Approvals:
jenkins-bot: Verified
Phuedx: Looks good to me, approved
diff --git a/resources/ext.popups/actions.js b/resources/ext.popups/actions.js
index e51c4b3..95389b3 100644
--- a/resources/ext.popups/actions.js
+++ b/resources/ext.popups/actions.js
@@ -48,25 +48,30 @@
* Represents Page Previews fetching data via the
[gateway](./gateway.js).
*
* @param {ext.popups.Gateway} gateway
- * @param {String} title
+ * @param {Element} el
* @return {Redux.Thunk}
*/
- function fetch( gateway, title ) {
+ function fetch( gateway, el ) {
+ var title = $( el ).data( 'page-previews-title' );
+
return function ( dispatch ) {
dispatch( {
type: types.FETCH_START,
+ el: el,
title: title
} );
gateway( title )
.fail( function () {
dispatch( {
- type: types.FETCH_FAILED
+ type: types.FETCH_FAILED,
+ el: el
} );
} )
.done( function ( result ) {
dispatch( {
type: types.FETCH_END,
+ el: el,
result: result
} );
} );
@@ -94,7 +99,7 @@
mw.popups.wait( FETCH_START_DELAY )
.then( function () {
if ( getState().preview.activeLink ===
el ) {
- dispatch( fetch( gateway, $( el
).data( 'page-previews-title' ) ) );
+ dispatch( fetch( gateway, el )
);
}
} );
};
diff --git a/resources/ext.popups/reducers.js b/resources/ext.popups/reducers.js
index 395ad7e..4c6aaee 100644
--- a/resources/ext.popups/reducers.js
+++ b/resources/ext.popups/reducers.js
@@ -44,7 +44,8 @@
linkInteractionToken: undefined,
activeLink: undefined,
activeEvent: undefined,
- interactionStarted: undefined
+ interactionStarted: undefined,
+ shouldShow: false
};
}
@@ -68,16 +69,22 @@
activeEvent: undefined,
interactionStarted: undefined,
linkInteractionToken: undefined,
- fetchResponse: undefined
+ fetchResponse: undefined,
+ shouldShow: false
} );
case mw.popups.actionTypes.FETCH_START:
return nextState( state, {
fetchResponse: undefined
} );
case mw.popups.actionTypes.FETCH_END:
- return nextState( state, {
- fetchResponse: action.result
- } );
+ if ( action.el === state.activeLink ) {
+ return nextState( state, {
+ fetchResponse: action.result,
+ shouldShow: true
+ } );
+ }
+
+ /* falls through */
default:
return state;
}
diff --git a/resources/ext.popups/renderChangeListener.js
b/resources/ext.popups/renderChangeListener.js
index 1288e35..f0c13a1 100644
--- a/resources/ext.popups/renderChangeListener.js
+++ b/resources/ext.popups/renderChangeListener.js
@@ -9,10 +9,10 @@
var preview;
return function ( prevState, state ) {
- if ( state.preview.fetchResponse && !preview ) {
+ if ( state.preview.shouldShow && !preview ) {
preview = mw.popups.renderer.render(
state.preview.fetchResponse );
preview.show( state.preview.activeEvent );
- } else if ( prevState &&
prevState.preview.fetchResponse ) {
+ } else if ( !state.preview.shouldShow && preview ) {
preview.hide()
.done( function () {
preview = undefined;
diff --git a/tests/qunit/ext.popups/actions.test.js
b/tests/qunit/ext.popups/actions.test.js
index 45b80f0..5f9af0b 100644
--- a/tests/qunit/ext.popups/actions.test.js
+++ b/tests/qunit/ext.popups/actions.test.js
@@ -54,23 +54,24 @@
gateway = function () {
return gatewayDeferred;
},
+ el = this.el,
fetchThunk,
result = {};
this.sandbox.stub( mw, 'now' ).returns( new Date() );
- mw.popups.actions.linkDwell( this.el, event, gateway )(
dispatch, this.getState );
+ mw.popups.actions.linkDwell( el, event, gateway )( dispatch,
this.getState );
assert.ok( dispatch.calledWith( {
type: 'LINK_DWELL',
- el: this.el,
+ el: el,
event: event,
interactionStarted: mw.now()
} ) );
// Stub the state tree being updated.
this.state.preview = {
- activeLink: this.el
+ activeLink: el
};
// ---
@@ -87,13 +88,17 @@
assert.ok( dispatch.calledWith( {
type: 'FETCH_START',
+ el: el,
title: 'Foo'
} ) );
+
+ // ---
gatewayDeferred.resolve( result );
assert.ok( dispatch.calledWith( {
type: 'FETCH_END',
+ el: el,
result: result
} ) );
diff --git a/tests/qunit/ext.popups/reducers.test.js
b/tests/qunit/ext.popups/reducers.test.js
index 4d2a590..5adf4d4 100644
--- a/tests/qunit/ext.popups/reducers.test.js
+++ b/tests/qunit/ext.popups/reducers.test.js
@@ -1,6 +1,10 @@
( function ( mw, $ ) {
- QUnit.module( 'ext.popups/reducers' );
+ QUnit.module( 'ext.popups/reducers', {
+ setup: function () {
+ this.el = $( '<a>' );
+ }
+ } );
QUnit.test( '#rootReducer', function ( assert ) {
var state = mw.popups.reducers.rootReducer( undefined, { type:
'@@INIT' } );
@@ -17,7 +21,8 @@
linkInteractionToken: undefined,
activeLink: undefined,
activeEvent: undefined,
- interactionStarted: undefined
+ interactionStarted: undefined,
+ shouldShow: false
},
renderer: {
isAnimating: false,
@@ -29,47 +34,106 @@
);
} );
- QUnit.test( '#preview', function ( assert ) {
- var state = mw.popups.reducers.preview( undefined, { type:
'@@INIT' } ),
- action;
-
- assert.expect( 2 );
-
- action = {
+ QUnit.test( '#preview: BOOT', function ( assert ) {
+ var action = {
type: 'BOOT',
isUserInCondition: true,
sessionToken: '0123456789',
pageToken: '9876543210'
};
+ assert.expect( 1 );
+
assert.deepEqual(
- mw.popups.reducers.preview( state, action ),
+ mw.popups.reducers.preview( {}, action ),
{
enabled: true,
sessionToken: '0123456789',
pageToken: '9876543210'
},
- 'It should set enabled and the session tokens on the
BOOT action'
+ 'It should set enabled and the session tokens.'
);
+ } );
- // ---
- action = {
+ QUnit.test( '#preview: LINK_DWELL', function ( assert ) {
+ var action = {
type: 'LINK_DWELL',
- el: $( '<a>' ),
+ el: this.el,
event: {},
interactionStarted: mw.now(),
linkInteractionToken: '0123456789'
};
assert.deepEqual(
- mw.popups.reducers.preview( state, action ),
+ mw.popups.reducers.preview( {}, action ),
{
activeLink: action.el,
activeEvent: action.event,
interactionStarted: action.interactionStarted,
linkInteractionToken:
action.linkInteractionToken
},
- 'It should set active link and event as well as
interaction info on the LINK_DWELL action'
+ 'It should set active link and event as well as
interaction info.'
+ );
+ } );
+
+ QUnit.test( '#preview: LINK_ABANDON', function ( assert ) {
+ var action = {
+ type: 'LINK_ABANDON',
+ el: this.el
+ };
+
+ assert.deepEqual(
+ mw.popups.reducers.preview( {}, action ),
+ {
+ activeLink: undefined,
+ activeEvent: undefined,
+ interactionStarted: undefined,
+ linkInteractionToken: undefined,
+ fetchResponse: undefined,
+ shouldShow: false
+ },
+ 'It should hide the preview and reset the interaction
info.'
+ );
+ } );
+
+ QUnit.test( '#preview: FETCH_END', function ( assert ) {
+ var state = {
+ activeLink: this.el
+ },
+ action = {
+ type: 'FETCH_END',
+ el: this.el,
+ result: {}
+ };
+
+ assert.expect( 2 );
+
+ assert.deepEqual(
+ mw.popups.reducers.preview( state, action ),
+ {
+ activeLink: state.activeLink, // Previous state.
+
+ fetchResponse: action.result,
+ shouldShow: true
+ },
+ 'It should store the result and signal that a preview
should be rendered.'
+ );
+
+ // ---
+
+ state = {
+ activeLink: $( '<a>' )
+ };
+ action = {
+ type: 'FETCH_END',
+ el: this.el,
+ result: {}
+ };
+
+ assert.deepEqual(
+ mw.popups.reducers.preview( state, action ),
+ state,
+ 'It should NOOP if the user has interacted with another
link since the request was dispatched via the gateway.'
);
} );
--
To view, visit https://gerrit.wikimedia.org/r/323524
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3dd1c0c566ec63de515174c14845d7927583ce93
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Popups
Gerrit-Branch: mpga
Gerrit-Owner: Phuedx <[email protected]>
Gerrit-Reviewer: Phuedx <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
