From d834329e15d1a77ed107d4d7da9166f1675958c2 Mon Sep 17 00:00:00 2001 From: jdlrobson Date: Fri, 3 Dec 2021 11:19:27 -0800 Subject: [PATCH] [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 --- resources/skins.vector.es6/AB.js | 12 ++++ resources/skins.vector.es6/main.js | 10 +-- resources/skins.vector.es6/scrollObserver.js | 44 +------------ resources/skins.vector.es6/stickyHeader.js | 69 +++++++++++++------- 4 files changed, 63 insertions(+), 72 deletions(-) diff --git a/resources/skins.vector.es6/AB.js b/resources/skins.vector.es6/AB.js index 39a92ae..0371a3b 100644 --- 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 }; diff --git a/resources/skins.vector.es6/main.js b/resources/skins.vector.es6/main.js index 6d538eb..48f08e2 100644 --- 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' ); } diff --git a/resources/skins.vector.es6/scrollObserver.js b/resources/skins.vector.es6/scrollObserver.js index 2b7e3dd..cbb1305 100644 --- 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 }; diff --git a/resources/skins.vector.es6/stickyHeader.js b/resources/skins.vector.es6/stickyHeader.js index 245d139..185fcc5 100644 --- 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,