[Sticky header refactor] Separate responsibilities

Move A/B test code to AB.js Consolidate the show/hide code spread across scrollObserver and stickyHeader by adding a show and hide function. This is needed to fix T296680 Change-Id: Ia2e0c50278df0dfc1600610f281be20f4cc755c2
2021-12-03 11:19:27 -08:00 · 2021-12-03 11:19:27 -08:00 · d834329e15
parent 2bccc4e8d4
commit d834329e15
4 changed files with 63 additions and 72 deletions
--- a/resources/skins.vector.es6/AB.js
+++ b/resources/skins.vector.es6/AB.js
@ -82,6 +82,17 @@ function getEnabledExperiment() {
 	return mergedConfig;
 }

+/**
+ * Determine if user is in test group to experience feature.
+ *
+ * @param {string} bucket the bucket name the user is assigned
+ * @param {string} targetGroup the target test group to experience feature
+ * @return {boolean} true if the user should experience feature
+ */
+function isInTestGroup( bucket, targetGroup ) {
+	return bucket === targetGroup;
+}
+
 /**
 * Fire hook to register A/B test enrollment.
 *
@ -103,6 +114,7 @@ function initAB( bucket ) {
 }

 module.exports = {
+	isInTestGroup,
 	getEnabledExperiment,
 	initAB
 };
--- a/resources/skins.vector.es6/main.js
+++ b/resources/skins.vector.es6/main.js
@ -18,6 +18,7 @@ const main = () => {

 	// Get the A/B test config for sticky header if enabled.
 	const
+		FEATURE_TEST_GROUP = 'stickyHeaderEnabled',
 		testConfig = AB.getEnabledExperiment(),
 		stickyConfig = testConfig &&
 			// @ts-ignore
@ -25,10 +26,11 @@ const main = () => {
 			testConfig : null,
 		// Note that the default test group is set to experience the feature by default.
 		// @ts-ignore
-		testGroup = stickyConfig ? stickyConfig.group : scrollObserver.FEATURE_TEST_GROUP,
+		testGroup = stickyConfig ? stickyConfig.group : FEATURE_TEST_GROUP,
 		targetElement = stickyHeader.header,
 		targetIntersection = stickyHeader.stickyIntersection,
-		isStickyHeaderAllowed = stickyHeader.isStickyHeaderAllowed() && testGroup !== 'unsampled';
+		isStickyHeaderAllowed = stickyHeader.isStickyHeaderAllowed() &&
+			testGroup !== 'unsampled' && AB.isInTestGroup( testGroup, FEATURE_TEST_GROUP );

 	// Fire the A/B test enrollment hook.
 	AB.initAB( testGroup );
@ -38,13 +40,13 @@ const main = () => {
 	const observer = scrollObserver.initScrollObserver(
 		() => {
 			if ( targetElement && isStickyHeaderAllowed ) {
-				scrollObserver.onShowFeature( targetElement, testGroup );
+				stickyHeader.show();
 			}
 			scrollObserver.fireScrollHook( 'down' );
 		},
 		() => {
 			if ( targetElement && isStickyHeaderAllowed ) {
-				scrollObserver.onHideFeature( targetElement, testGroup );
+				stickyHeader.hide();
 			}
 			scrollObserver.fireScrollHook( 'up' );
 		}
--- a/resources/skins.vector.es6/scrollObserver.js
+++ b/resources/skins.vector.es6/scrollObserver.js
@ -1,48 +1,9 @@
 const
-	FEATURE_VISIBLE_CLASS = 'vector-sticky-header-visible',
-	FEATURE_TEST_GROUP = 'stickyHeaderEnabled',
 	SCROLL_HOOK = 'vector.page_title_scroll',
 	SCROLL_CONTEXT_ABOVE = 'scrolled-above-page-title',
 	SCROLL_CONTEXT_BELOW = 'scrolled-below-page-title',
 	SCROLL_ACTION = 'scroll-to-top';

-/**
- * Determine if user is in test group to experience feature.
- *
- * @param {string} bucket the bucket name the user is assigned
- * @param {string} targetGroup the target test group to experience feature
- * @return {boolean} true if the user should experience feature
- */
-function isInTestGroup( bucket, targetGroup ) {
-	return bucket === targetGroup;
-}
-
-/**
- * Show the feature based on test group.
- *
- * @param {HTMLElement} element target feature
- * @param {string} group A/B test bucket of the user
- */
-function onShowFeature( element, group ) {
-	if ( isInTestGroup( group, FEATURE_TEST_GROUP ) ) {
-		// eslint-disable-next-line mediawiki/class-doc
-		element.classList.add( FEATURE_VISIBLE_CLASS );
-	}
-}
-
-/**
- * Hide the feature based on test group.
- *
- * @param {HTMLElement} element target feature
- * @param {string} group A/B test bucket of the user
- */
-function onHideFeature( element, group ) {
-	if ( isInTestGroup( group, FEATURE_TEST_GROUP ) ) {
-		// eslint-disable-next-line mediawiki/class-doc
-		element.classList.remove( FEATURE_VISIBLE_CLASS );
-	}
-}
-
 /**
 * Fire a hook to be captured by WikimediaEvents for scroll event logging.
 *
@ -83,8 +44,5 @@ function initScrollObserver( show, hide ) {

 module.exports = {
 	initScrollObserver,
-	onShowFeature,
-	onHideFeature,
-	fireScrollHook,
-	FEATURE_TEST_GROUP
+	fireScrollHook
 };
--- a/resources/skins.vector.es6/stickyHeader.js
+++ b/resources/skins.vector.es6/stickyHeader.js
@ -3,6 +3,7 @@
 */
 const
 	STICKY_HEADER_ID = 'vector-sticky-header',
+	header = document.getElementById( STICKY_HEADER_ID ),
 	initSearchToggle = require( './searchToggle.js' ),
 	STICKY_HEADER_APPENDED_ID = '-sticky-header',
 	STICKY_HEADER_VISIBLE_CLASS = 'vector-sticky-header-visible',
@ -27,6 +28,26 @@ function copyAttribute( from, to, attribute ) {
 	}
 }

+/**
+ * Show the sticky header.
+ */
+function show() {
+	if ( header ) {
+		// eslint-disable-next-line mediawiki/class-doc
+		header.classList.add( STICKY_HEADER_VISIBLE_CLASS );
+	}
+}
+
+/**
+ * Hide the sticky header.
+ */
+function hide() {
+	if ( header ) {
+		// eslint-disable-next-line mediawiki/class-doc
+		header.classList.remove( STICKY_HEADER_VISIBLE_CLASS );
+	}
+}
+
 /**
 * Copies attribute from an element to another.
 *
@ -93,13 +114,13 @@ function removeClassFromNodes( nodes, className ) {
 /**
 * Makes sticky header icons functional for modern Vector.
 *
- * @param {HTMLElement} header
+ * @param {HTMLElement} headerElement
 * @param {HTMLElement|null} history
 * @param {HTMLElement|null} talk
 */
-function prepareIcons( header, history, talk ) {
-	const historySticky = header.querySelector( '#ca-history-sticky-header' ),
-		talkSticky = header.querySelector( '#ca-talk-sticky-header' );
+function prepareIcons( headerElement, history, talk ) {
+	const historySticky = headerElement.querySelector( '#ca-history-sticky-header' ),
+		talkSticky = headerElement.querySelector( '#ca-talk-sticky-header' );

 	if ( !historySticky || !talkSticky ) {
 		throw new Error( 'Sticky header has unexpected HTML' );
@ -122,7 +143,7 @@ function prepareIcons( header, history, talk ) {
 /**
 * Render sticky header edit or protected page icons for modern Vector.
 *
- * @param {HTMLElement} header
+ * @param {HTMLElement} headerElement
 * @param {HTMLElement|null} primaryEdit
 * @param {boolean} isProtected
 * @param {HTMLElement|null} secondaryEdit
@ -130,28 +151,28 @@ function prepareIcons( header, history, talk ) {
 *  header.
 */
 function prepareEditIcons(
-	header,
+	headerElement,
 	primaryEdit,
 	isProtected,
 	secondaryEdit,
 	disableStickyHeader
 ) {
 	const
-		primaryEditStickyElement = header.querySelector(
+		primaryEditStickyElement = headerElement.querySelector(
 			'#ca-ve-edit-sticky-header'
 		),
 		primaryEditSticky = primaryEditStickyElement ? toHTMLElement(
-			header.querySelector(
+			headerElement.querySelector(
 				'#ca-ve-edit-sticky-header'
 			)
 		) : null,
 		protectedSticky = toHTMLElement(
-			header.querySelector(
+			headerElement.querySelector(
 				'#ca-viewsource-sticky-header'
 			)
 		),
 		wikitextSticky = toHTMLElement(
-			header.querySelector(
+			headerElement.querySelector(
 				'#ca-edit-sticky-header'
 			)
 		);
@ -229,15 +250,13 @@ function isInViewport( element ) {
 /**
 * Add hooks for sticky header when Visual Editor is used.
 *
- * @param {HTMLElement} element target feature
 * @param {HTMLElement} targetIntersection intersection element
 * @param {IntersectionObserver} observer
 */
-function addVisualEditorHooks( element, targetIntersection, observer ) {
+function addVisualEditorHooks( targetIntersection, observer ) {
 	// When Visual Editor is activated, hide the sticky header.
 	mw.hook( 've.activate' ).add( () => {
-		// eslint-disable-next-line mediawiki/class-doc
-		element.classList.remove( STICKY_HEADER_VISIBLE_CLASS );
+		hide();
 		observer.unobserve( targetIntersection );
 	} );

@ -250,8 +269,7 @@ function addVisualEditorHooks( element, targetIntersection, observer ) {
 	// After saving edits, re-apply the sticky header if the target is not in the viewport.
 	mw.hook( 'postEdit.afterRemoval' ).add( () => {
 		if ( !isInViewport( targetIntersection ) ) {
-			// eslint-disable-next-line mediawiki/class-doc
-			element.classList.add( STICKY_HEADER_VISIBLE_CLASS );
+			show();
 			observer.observe( targetIntersection );
 		}
 	} );
@ -260,14 +278,14 @@ function addVisualEditorHooks( element, targetIntersection, observer ) {
 /**
 * Makes sticky header functional for modern Vector.
 *
- * @param {HTMLElement} header
+ * @param {HTMLElement} headerElement
 * @param {HTMLElement} userMenu
 * @param {Element} userMenuStickyContainer
 * @param {IntersectionObserver} stickyObserver
 * @param {HTMLElement} stickyIntersection
 */
 function makeStickyHeaderFunctional(
-	header,
+	headerElement,
 	userMenu,
 	userMenuStickyContainer,
 	stickyObserver,
@ -305,7 +323,7 @@ function makeStickyHeaderFunctional(
 		userMenuStickyContainerInner.appendChild( userMenuClone );
 	}

-	prepareIcons( header,
+	prepareIcons( headerElement,
 		document.querySelector( '#ca-history a' ),
 		document.querySelector( '#ca-talk a' )
 	);
@ -318,12 +336,12 @@ function makeStickyHeaderFunctional(
 	const secondaryEdit = veEdit ? ceEdit : null;
 	const disableStickyHeader = () => {
 		// eslint-disable-next-line mediawiki/class-doc
-		header.classList.remove( STICKY_HEADER_VISIBLE_CLASS );
+		headerElement.classList.remove( STICKY_HEADER_VISIBLE_CLASS );
 		stickyObserver.unobserve( stickyIntersection );
 	};

 	prepareEditIcons(
-		header,
+		headerElement,
 		toHTMLElement( primaryEdit ),
 		isProtected,
 		toHTMLElement( secondaryEdit ),
@ -334,11 +352,11 @@ function makeStickyHeaderFunctional(
 }

 /**
- * @param {HTMLElement} header
+ * @param {HTMLElement} headerElement
 */
-function setupSearchIfNeeded( header ) {
+function setupSearchIfNeeded( headerElement ) {
 	const
-		searchToggle = header.querySelector( SEARCH_TOGGLE_SELECTOR );
+		searchToggle = headerElement.querySelector( SEARCH_TOGGLE_SELECTOR );

 	if ( !document.body.classList.contains( 'skin-vector-search-vue' ) ) {
 		return;
@ -374,7 +392,6 @@ function isAllowedAction( action ) {
 }

 const
-	header = document.getElementById( STICKY_HEADER_ID ),
 	stickyIntersection = document.getElementById(
 		FIRST_HEADING_ID
 	),
@ -424,6 +441,8 @@ function initStickyHeader( observer ) {
 }

 module.exports = {
+	show,
+	hide,
 	initStickyHeader,
 	isStickyHeaderAllowed,
 	header,