From 97de09dcbaa49d5a216bc2f918e468bfc02ec60b Mon Sep 17 00:00:00 2001 From: bwang Date: Tue, 13 Dec 2022 14:28:22 -0600 Subject: [PATCH] Refactor page tools, main menu, and TOC components - getTocData is moved into VectorComponentTableOfContents and it's test file The following changes were made to the main menu, toc and page tools PHP components - Avoid passing in $skin to the constructor - Handle isPinned logic inside the component - Add a public ID constant to the components - Dropdown data for each feature use the same naming convention Bug: T317900 Change-Id: I77a617a6c1d93bccd3b6e59353299f5534624e53 --- .../Components/VectorComponentMainMenu.php | 45 +++-- .../Components/VectorComponentPageTools.php | 24 +-- .../VectorComponentTableOfContents.php | 81 ++++++++- includes/FeatureManagement/FeatureManager.php | 3 +- includes/SkinVector.php | 8 +- includes/SkinVector22.php | 136 +++++--------- includes/templates/Menu.mustache | 2 +- includes/templates/UserLinks.mustache | 5 +- resources/skins.vector.es6/tableOfContents.js | 3 +- .../components/MainMenuDropdown.less | 7 - .../jest/__snapshots__/userLinks.test.js.snap | 1 - tests/jest/userLinksData.js | 2 +- tests/phpunit/integration/SkinVectorTest.php | 157 ---------------- .../VectorComponentPageToolsTest.php | 132 ++++++++++++++ .../VectorComponentTableOfContentsTest.php | 171 +++++++++++++++++- 15 files changed, 467 insertions(+), 310 deletions(-) create mode 100644 tests/phpunit/unit/components/VectorComponentPageToolsTest.php diff --git a/includes/Components/VectorComponentMainMenu.php b/includes/Components/VectorComponentMainMenu.php index 3ed52dd06..958b62642 100644 --- a/includes/Components/VectorComponentMainMenu.php +++ b/includes/Components/VectorComponentMainMenu.php @@ -19,55 +19,52 @@ class VectorComponentMainMenu implements VectorComponent { private $languageData; /** @var MessageLocalizer */ private $localizer; - /** @var User */ - private $user; + /** @var VectorComponentPinnableHeader|null */ + private $pinnableHeader; + /** @var string */ + public const ID = 'vector-main-menu'; /** * @param array $sidebarData - * @param Skin $skin * @param bool $shouldLanguageAlertBeInSidebar * @param array $languageData + * @param MessageLocalizer $localizer + * @param User $user + * @param Skin $skin */ public function __construct( array $sidebarData, - Skin $skin, bool $shouldLanguageAlertBeInSidebar, - array $languageData + array $languageData, + MessageLocalizer $localizer, + User $user, + Skin $skin ) { $this->sidebarData = $sidebarData; - $this->localizer = $skin->getContext(); $this->languageData = $languageData; - $user = $skin->getUser(); - $this->user = $user; + $this->localizer = $localizer; + if ( $user->isRegistered() ) { $this->optOut = new VectorComponentMainMenuActionOptOut( $skin ); + $this->pinnableHeader = new VectorComponentPinnableHeader( + $this->localizer, + false, + self::ID, + null + ); } if ( $shouldLanguageAlertBeInSidebar ) { $this->alert = new VectorComponentMainMenuActionLanguageSwitchAlert( $skin ); } } - /** - * @return User - */ - private function getUser(): User { - return $this->user; - } - /** * @inheritDoc */ public function getTemplateData(): array { $action = $this->optOut; $alert = $this->alert; - - $id = 'vector-main-menu'; - $pinnableHeader = new VectorComponentPinnableHeader( - $this->localizer, - false, - $id, - null - ); + $pinnableHeader = $this->pinnableHeader; $portletsRest = []; foreach ( $this->sidebarData[ 'array-portlets-rest' ] as $data ) { @@ -81,7 +78,7 @@ class VectorComponentMainMenu implements VectorComponent { 'data-main-menu-action' => $action ? $action->getTemplateData() : null, // T295555 Add language switch alert message temporarily (to be removed). 'data-vector-language-switch-alert' => $alert ? $alert->getTemplateData() : null, - 'data-pinnable-header' => $pinnableHeader->getTemplateData(), + 'data-pinnable-header' => $pinnableHeader ? $pinnableHeader->getTemplateData() : null, 'data-languages' => $languageMenu->getTemplateData(), ]; } diff --git a/includes/Components/VectorComponentPageTools.php b/includes/Components/VectorComponentPageTools.php index d3f47825f..442d56e90 100644 --- a/includes/Components/VectorComponentPageTools.php +++ b/includes/Components/VectorComponentPageTools.php @@ -1,6 +1,8 @@ menus = $menus; - $this->isPinned = $isPinned; $this->localizer = $localizer; + $this->isPinned = $featureManager->isFeatureEnabled( Constants::FEATURE_PAGE_TOOLS_PINNED ); $this->pinnableHeader = $user->isRegistered() ? new VectorComponentPinnableHeader( $localizer, - $isPinned, + $this->isPinned, // Name - 'vector-page-tools', + self::ID, // Feature name 'page-tools-pinned' ) : null; @@ -64,10 +66,10 @@ class VectorComponentPageTools implements VectorComponent { return array_map( function ( $menu ) { switch ( $menu['id'] ?? '' ) { case self::TOOLBOX_ID: - $menu['label'] = $this->localizer->msg( 'vector-page-tools-general-label' ); + $menu['label'] = $this->localizer->msg( 'vector-page-tools-general-label' )->text(); break; case self::ACTIONS_ID: - $menu['label'] = $this->localizer->msg( 'vector-page-tools-actions-label' ); + $menu['label'] = $this->localizer->msg( 'vector-page-tools-actions-label' )->text(); break; } diff --git a/includes/Components/VectorComponentTableOfContents.php b/includes/Components/VectorComponentTableOfContents.php index cfa1469fe..9bcac981d 100644 --- a/includes/Components/VectorComponentTableOfContents.php +++ b/includes/Components/VectorComponentTableOfContents.php @@ -1,18 +1,91 @@ tocData = $tocData; + $this->localizer = $localizer; + // ToC is pinned by default, hardcoded for now + $this->isPinned = true; + $this->config = $config; + $this->pinnableHeader = new VectorComponentPinnableHeader( + $this->localizer, + $this->isPinned, + 'vector-toc', + null, + false, + 'h2' + ); + } + + /** + * In tableOfContents.js we have tableOfContents::getTableOfContentsSectionsData(), + * that yields the same result as this function, please make sure to keep them in sync. * @inheritDoc */ public function getTemplateData(): array { - $pinnableElementName = 'vector-toc'; - $pinnedContainer = new VectorComponentPinnedContainer( $pinnableElementName ); - $pinnableElement = new VectorComponentPinnableElement( $pinnableElementName ); - return $pinnableElement->getTemplateData() + $pinnedContainer->getTemplateData(); + // If the table of contents has no items, we won't output it. + // empty array is interpreted by Mustache as falsey. + if ( empty( $this->tocData ) || empty( $this->tocData[ 'array-sections' ] ) ) { + return []; + } + + // Populate button labels for collapsible TOC sections + foreach ( $this->tocData[ 'array-sections' ] as &$section ) { + if ( $section['is-top-level-section'] && $section['is-parent-section'] ) { + $section['vector-button-label'] = + $this->localizer->msg( 'vector-toc-toggle-button-label', $section['line'] )->text(); + } + } + + $pinnedContainer = new VectorComponentPinnedContainer( self::ID ); + $pinnableElement = new VectorComponentPinnableElement( self::ID ); + + return $pinnableElement->getTemplateData() + + $pinnedContainer->getTemplateData() + + array_merge( $this->tocData, [ + 'is-vector-toc-beginning-enabled' => $this->config->get( + 'VectorTableOfContentsBeginning' + ), + 'vector-is-collapse-sections-enabled' => + $this->tocData[ 'number-section-count'] >= $this->config->get( + 'VectorTableOfContentsCollapseAtCount' + ), + 'data-pinnable-header' => $this->pinnableHeader->getTemplateData(), + ] ); } } diff --git a/includes/FeatureManagement/FeatureManager.php b/includes/FeatureManagement/FeatureManager.php index 497fbeefd..b68bf793d 100644 --- a/includes/FeatureManagement/FeatureManager.php +++ b/includes/FeatureManagement/FeatureManager.php @@ -35,8 +35,9 @@ use Wikimedia\Assert\Assert; * * @package MediaWiki\Skins\Vector\FeatureManagement * @internal + * @final */ -final class FeatureManager { +class FeatureManager { /** * A map of feature name to the array of requirements (referenced by name). A feature is only diff --git a/includes/SkinVector.php b/includes/SkinVector.php index a07c79966..138e307b0 100644 --- a/includes/SkinVector.php +++ b/includes/SkinVector.php @@ -285,7 +285,7 @@ abstract class SkinVector extends SkinMustache { $returnto = $this->getReturnToParam(); $useCombinedLoginLink = $this->useCombinedLoginLink(); $userMenuOverflowData = Hooks::updateDropdownMenuData( $overflowMenuData ); - $userMenu = $this->getUserMenuDropdown( $userMenuData ); + $userMenuDropdown = $this->getUserMenuDropdown( $userMenuData ); unset( $userMenuOverflowData[ 'label' ] ); if ( $isAnon || $isTempUser ) { @@ -311,7 +311,7 @@ abstract class SkinVector extends SkinMustache { 'is-temp-user' => $isTempUser, 'is-wide' => $moreItems > 3, 'data-user-menu-overflow' => $userMenuOverflowData, - 'data-user-menu' => $userMenu->getTemplateData(), + 'data-user-menu-dropdown' => $userMenuDropdown->getTemplateData(), 'html-items' => $userMenuData['html-items'], ]; } @@ -371,7 +371,7 @@ abstract class SkinVector extends SkinMustache { } $btns[] = $this->getAddSectionButtonData(); - // FIXME: Sync with SkinVector22:getTocData + // FIXME: Sync with VectorComponentTableOfContents:getTemplateData $tocPortletData = Hooks::updateDropdownMenuData( [ 'id' => 'vector-sticky-header-toc', 'class' => 'mw-portlet mw-portlet-sticky-header-toc vector-sticky-header-toc', @@ -530,7 +530,7 @@ abstract class SkinVector extends SkinMustache { // completion of T319356. // // Also, add target class to apply different icon to personal menu dropdown for logged in users. - $portletData['class'] = 'mw-portlet mw-portlet-personal vector-user-menu vector-menu-dropdown'; + $portletData['class'] = 'mw-portlet mw-portlet-personal vector-user-menu'; $portletData['class'] .= $this->loggedin ? ' vector-user-menu-logged-in' : ' vector-user-menu-logged-out'; diff --git a/includes/SkinVector22.php b/includes/SkinVector22.php index af68673aa..6a76eb1d0 100644 --- a/includes/SkinVector22.php +++ b/includes/SkinVector22.php @@ -6,7 +6,6 @@ use MediaWiki\MediaWikiServices; use MediaWiki\Skins\Vector\Components\VectorComponentDropdown; use MediaWiki\Skins\Vector\Components\VectorComponentMainMenu; use MediaWiki\Skins\Vector\Components\VectorComponentPageTools; -use MediaWiki\Skins\Vector\Components\VectorComponentPinnableHeader; use MediaWiki\Skins\Vector\Components\VectorComponentSearchBox; use MediaWiki\Skins\Vector\Components\VectorComponentStickyHeader; use MediaWiki\Skins\Vector\Components\VectorComponentTableOfContents; @@ -45,55 +44,6 @@ class SkinVector22 extends SkinVector { $shouldShowOnMainPage; } - /** - * Annotates table of contents data with Vector-specific information. - * - * In tableOfContents.js we have tableOfContents::getTableOfContentsSectionsData(), - * that yields the same result as this function, please make sure to keep them in sync. - * FIXME: This code should be moved to VectorComponentTableOfContents. - * - * @param array $tocData - * @return array - */ - private function getTocData( array $tocData ): array { - // If the table of contents has no items, we won't output it. - // empty array is interpreted by Mustache as falsey. - if ( empty( $tocData ) || empty( $tocData[ 'array-sections' ] ) ) { - return []; - } - - // Populate button labels for collapsible TOC sections - foreach ( $tocData[ 'array-sections' ] as &$section ) { - if ( $section['is-top-level-section'] && $section['is-parent-section'] ) { - $section['vector-button-label'] = - $this->msg( 'vector-toc-toggle-button-label', $section['line'] )->text(); - } - } - - // ToC is pinned by default, hardcoded for now - $isTocPinned = true; - $pinnableHeader = new VectorComponentPinnableHeader( - $this->getContext(), - $isTocPinned, - 'vector-toc', - null, - false, - 'h2' - ); - - return array_merge( $tocData, [ - 'is-vector-toc-beginning-enabled' => $this->getConfig()->get( - 'VectorTableOfContentsBeginning' - ), - 'vector-is-collapse-sections-enabled' => - $tocData[ 'number-section-count'] >= $this->getConfig()->get( - 'VectorTableOfContentsCollapseAtCount' - ), - 'is-pinned' => $isTocPinned, - 'data-pinnable-header' => $pinnableHeader->getTemplateData(), - ] ); - } - /** * @return array */ @@ -188,11 +138,6 @@ class SkinVector22 extends SkinVector { $featureManager = VectorServices::getFeatureManager(); $parentData = parent::getTemplateData(); $stickyHeader = new VectorComponentStickyHeader(); - $toc = new VectorComponentTableOfContents(); - $tocData = $parentData['data-toc'] ?? []; - $parentData['data-toc'] = !empty( $tocData ) ? - $toc->getTemplateData() + $this->getTocData( $tocData ) : null; - $parentData = $this->mergeViewOverflowIntoActions( $parentData ); // FIXME: Move to component (T322089) @@ -214,8 +159,26 @@ class SkinVector22 extends SkinVector { ); } + $toc = new VectorComponentTableOfContents( + $parentData['data-toc'], + $this->getContext(), + $this->getConfig() + ); + + $config = $this->getConfig(); + $searchBox = new VectorComponentSearchBox( + $parentData['data-search-box'], + true, + // is primary mode of search + true, + 'searchform', + true, + $config, + Constants::SEARCH_BOX_INPUT_LOCATION_MOVED, + $this->getContext() + ); + $isPageToolsEnabled = $featureManager->isFeatureEnabled( Constants::FEATURE_PAGE_TOOLS ); - $isPageToolsPinned = $featureManager->isFeatureEnabled( Constants::FEATURE_PAGE_TOOLS_PINNED ); $sidebar = $parentData[ 'data-portlets-sidebar' ]; $pageToolsMenus = []; $restPortlets = $parentData[ 'data-portlets-sidebar' ][ 'array-portlets-rest' ]; @@ -236,43 +199,42 @@ class SkinVector22 extends SkinVector { $sidebar = $parentData[ 'data-portlets-sidebar' ]; } } - $config = $this->getConfig(); - $mainMenuDropdownClass = $this->getUser()->isAnon() ? 'vector-main-menu-btn-dropdown-anon ' : ''; - $mainMenuDropdownClass .= 'vector-main-menu-dropdown'; + + $isRegistered = $this->getUser()->isRegistered(); + $mainMenu = new VectorComponentMainMenu( + $sidebar, + $this->shouldLanguageAlertBeInSidebar(), + $parentData['data-portlets']['data-languages'] ?? [], + $this->getContext(), + $this->getUser(), + $this, + ); $mainMenuDropdown = new VectorComponentDropdown( - 'vector-main-menu-dropdown', - $this->msg( 'vector-main-menu-label' )->text(), - $mainMenuDropdownClass, + $mainMenu::ID . '-dropdown', + $this->msg( $mainMenu::ID . '-label' )->text(), + $mainMenu::ID . '-dropdown', 'menu' ); + $pageTools = new VectorComponentPageTools( + array_merge( [ $parentData['data-portlets']['data-actions'] ?? [] ], $pageToolsMenus ), + $this->getContext(), + $this->getUser(), + VectorServices::getFeatureManager() + ); + $pageToolsDropdown = new VectorComponentDropdown( + $pageTools::ID . '-dropdown', + $this->msg( 'toolbox' )->text(), + $pageTools::ID . '-dropdown', + ); + $components = [ - 'data-search-box' => new VectorComponentSearchBox( - $parentData['data-search-box'], - true, - // is primary mode of search - true, - 'searchform', - true, - $config, - Constants::SEARCH_BOX_INPUT_LOCATION_MOVED, - $this->getContext() - ), + 'data-toc' => $toc, + 'data-search-box' => $searchBox, + 'data-portlets-main-menu' => $mainMenu, 'data-main-menu-dropdown' => $mainMenuDropdown, - 'data-portlets-main-menu' => new VectorComponentMainMenu( - $sidebar, - $this, - $this->shouldLanguageAlertBeInSidebar(), - $parentData['data-portlets']['data-languages'] ?? [], - ), - 'data-page-tools' => $isPageToolsEnabled ? new VectorComponentPageTools( - array_merge( [ $parentData['data-portlets']['data-actions'] ?? [] ], $pageToolsMenus ), - $isPageToolsPinned, - $this->getContext(), - $this->getUser() - ) : null, - 'data-page-tools-dropdown' => $isPageToolsEnabled ? - new VectorComponentDropdown( 'vector-page-tools', $this->msg( 'toolbox' )->text() ) : null, + 'data-page-tools' => $isPageToolsEnabled ? $pageTools : null, + 'data-page-tools-dropdown' => $isPageToolsEnabled ? $pageToolsDropdown : null, ]; foreach ( $components as $key => $component ) { // Array of components or null values. diff --git a/includes/templates/Menu.mustache b/includes/templates/Menu.mustache index 7f3b543e2..3a212e1bd 100644 --- a/includes/templates/Menu.mustache +++ b/includes/templates/Menu.mustache @@ -5,7 +5,7 @@
{{#label}}
- {{{.}}} + {{.}}
{{/label}} {{>MenuContents}} diff --git a/includes/templates/UserLinks.mustache b/includes/templates/UserLinks.mustache index d33e420f5..2c962a006 100644 --- a/includes/templates/UserLinks.mustache +++ b/includes/templates/UserLinks.mustache @@ -1,7 +1,6 @@
{{/is-anon}} {{>Dropdown/Close}} - {{/data-user-menu}} + {{/data-user-menu-dropdown}} diff --git a/resources/skins.vector.es6/tableOfContents.js b/resources/skins.vector.es6/tableOfContents.js index 7756d6bc0..b4183220e 100644 --- a/resources/skins.vector.es6/tableOfContents.js +++ b/resources/skins.vector.es6/tableOfContents.js @@ -478,7 +478,8 @@ module.exports = function tableOfContents( props ) { /** * Prepares the data for rendering the table of contents, * nesting child sections within their parent sections. - * This shoul yield the same result as the php function SkinVector22::getTocData(), + * This should yield the same result as the php function + * VectorComponentTableOfContents::getTemplateData(), * please make sure to keep them in sync. * * @param {Section[]} sections diff --git a/resources/skins.vector.styles/components/MainMenuDropdown.less b/resources/skins.vector.styles/components/MainMenuDropdown.less index fdd30fb31..3c1ea404c 100644 --- a/resources/skins.vector.styles/components/MainMenuDropdown.less +++ b/resources/skins.vector.styles/components/MainMenuDropdown.less @@ -22,10 +22,3 @@ display: none; } } - -// For anons the menu is not pinnable. -.vector-main-menu-btn-dropdown-anon { - .vector-pinnable-header { - display: none; - } -} diff --git a/tests/jest/__snapshots__/userLinks.test.js.snap b/tests/jest/__snapshots__/userLinks.test.js.snap index 6ea9a5825..f8c8c0111 100644 --- a/tests/jest/__snapshots__/userLinks.test.js.snap +++ b/tests/jest/__snapshots__/userLinks.test.js.snap @@ -19,7 +19,6 @@ exports[`UserLinks renders 1`] = ` -