From 9f1a1fa829fb4b7fc12966d47507b7d941706388 Mon Sep 17 00:00:00 2001 From: jdlrobson Date: Fri, 4 Sep 2020 12:18:57 -0700 Subject: [PATCH] Simplify menu code SkinMustache in core provides most of what is required for Vector to generate its menus. In the interest of having a canonical source of truth for menus across all skins, Vector should use this data. To ensure the HTML generated is (mostly) the same after this patch to prior, a few modifications are necessary: * The data from core is decorated so that Vector can continue having its own custom class names on menus. This is done using the decoratePortletClass method. * There is no support for a menu having a header representing the selected menu item, as is currently the case with variants. This is achieved via an extension to getPortletData. It's assumed that later when variants are merged with languages, this can be removed. * Menus are agnostic to how they are displayed, so we must continue to add the is-dropdown template variable to drop down menus. In future we may want to rethink our Menu partial to make this unnecessary in PHP. * The portal-first class is redundant in the modern Vector as we can use the first-child selector. Previously we introduced a class to service the legacy skin where this rule doesn't apply as #p-logo is the first child. However, the legacy skin can do this using a special next sibling selector instead. Bug: T268157 Change-Id: I5f7adc1840441b508ffee40139b85b64021789e6 --- includes/SkinVector.php | 178 +++++------------- includes/templates/Header.mustache | 4 +- includes/templates/Navigation.mustache | 4 +- includes/templates/Sidebar.mustache | 10 +- includes/templates/legacy/Sidebar.mustache | 10 +- includes/templates/skin-legacy.mustache | 24 +-- includes/templates/skin.mustache | 15 +- .../skins.vector.styles/legacy/Sidebar.less | 2 +- .../skins.vector.styles/legacy/layout.less | 5 + skin.json | 1 + stories/MenuPortal.stories.data.js | 2 +- stories/Sidebar.stories.data.js | 20 +- stories/skin.stories.data.js | 36 ++-- stories/types.js | 4 +- tests/phpunit/integration/SkinVectorTest.php | 15 +- 15 files changed, 131 insertions(+), 199 deletions(-) diff --git a/includes/SkinVector.php b/includes/SkinVector.php index f159c6e..11d9ad4 100644 --- a/includes/SkinVector.php +++ b/includes/SkinVector.php @@ -130,9 +130,8 @@ class SkinVector extends SkinMustache { 'input-location' => $this->getSearchBoxInputLocation(), - 'data-sidebar' => $this->getTemplateDataSidebar(), 'sidebar-visible' => $this->isSidebarVisible(), - ], $this->getMenuProps() ); + ] ); if ( $skin->getUser()->isRegistered() ) { // Note: This data is also passed to legacy template where it is unused. @@ -190,149 +189,70 @@ class SkinVector extends SkinMustache { } /** - * Render a series of portals - * - * @return array + * helper for applying Vector menu classes to portlets + * @param array $portletData returned by SkinMustache to decorate + * @param int $type representing one of the menu types (see MENU_TYPE_* constants) + * @return array modified version of portletData input */ - private function getTemplateDataSidebar() { - $skin = $this; - $portals = $this->buildSidebar(); - $props = []; - $languages = null; - - // Render portals - foreach ( $portals as $name => $content ) { - if ( $content === false ) { - continue; - } - - // Numeric strings gets an integer when set as key, cast back - T73639 - $name = (string)$name; - - switch ( $name ) { - case 'SEARCH': - break; - case 'TOOLBOX': - $props[] = $this->getMenuData( - 'tb', $content, self::MENU_TYPE_PORTAL - ); - break; - case 'LANGUAGES': - $portal = $this->getMenuData( - 'lang', $content, self::MENU_TYPE_PORTAL - ); - // The language portal will be added provided either - // languages exist or there is a value in html-after-portal - // for example to show the add language wikidata link (T252800) - if ( count( $content ) || $portal['html-after-portal'] ) { - $languages = $portal; - } - break; - default: - $props[] = $this->getMenuData( - $name, $content, self::MENU_TYPE_PORTAL - ); - break; - } - } - - $firstPortal = $props[0] ?? null; - if ( $firstPortal ) { - $firstPortal[ 'class' ] .= ' portal-first'; - } - - return [ - 'html-logo-attributes' => Xml::expandAttributes( - Linker::tooltipAndAccesskeyAttribs( 'p-logo' ) + [ - 'class' => 'mw-wiki-logo', - 'href' => Skin::makeMainPageUrl(), - ] - ), - 'array-portals-rest' => array_slice( $props, 1 ), - 'data-portals-first' => $firstPortal, - 'data-portals-languages' => $languages, - ]; - } - - /** - * @param string $label to be used to derive the id and human readable label of the menu - * Note certain keys are special cased for historic reasons in core. - * @param array $urls to convert to list items stored as string in html-items key - * @param int $type of menu (optional) - a plain list (MENU_TYPE_DEFAULT), - * a tab (MENU_TYPE_TABS) or a dropdown (MENU_TYPE_DROPDOWN) - * @param bool $setLabelToSelected (optional) the menu label will take the value of the - * selected item if found. - * @return array - */ - private function getMenuData( - string $label, - array $urls = [], - int $type = self::MENU_TYPE_DEFAULT, - bool $setLabelToSelected = false - ) : array { - $portletData = $this->getPortletData( $label, $urls ); + private function decoratePortletClass( + array $portletData, + int $type = self::MENU_TYPE_DEFAULT + ) { $extraClasses = [ self::MENU_TYPE_DROPDOWN => 'vector-menu vector-menu-dropdown', self::MENU_TYPE_TABS => 'vector-menu vector-menu-tabs', self::MENU_TYPE_PORTAL => 'vector-menu vector-menu-portal portal', self::MENU_TYPE_DEFAULT => 'vector-menu', ]; - $isPortal = $type === self::MENU_TYPE_PORTAL; + $class = $portletData['class']; + $portletData['class'] = trim( "$class $extraClasses[$type]" ); + return $portletData; + } - $props = $portletData + [ - 'label-id' => "p-{$label}-label", - 'is-dropdown' => $type === self::MENU_TYPE_DROPDOWN, - ]; + /** + * @inheritDoc + * @return array + */ + protected function getPortletData( + $label, + array $urls = [] + ) : array { + switch ( $label ) { + case 'actions': + case 'variants': + $type = self::MENU_TYPE_DROPDOWN; + break; + case 'views': + case 'namespaces': + $type = self::MENU_TYPE_TABS; + break; + case 'personal': + $type = self::MENU_TYPE_DEFAULT; + break; + default: + $type = self::MENU_TYPE_PORTAL; + break; + } + + $portletData = $this->decoratePortletClass( + parent::getPortletData( $label, $urls ), + $type + ); // Special casing for Variant to change label to selected. // Hopefully we can revisit and possibly remove this code when the language switcher is moved. - foreach ( $urls as $key => $item ) { - if ( $setLabelToSelected ) { + if ( $label === 'variants' ) { + foreach ( $urls as $key => $item ) { + // Check the class of the item for a `selected` class and if so, propagate the items + // label to the main label. if ( isset( $item['class'] ) && stripos( $item['class'], 'selected' ) !== false ) { - $props['label'] = $item['text']; + $portletData['label'] = $item['text']; } } } - // Mark the portal as empty if it has no content - $class = $props['class']; - $props['class'] = trim( "$class $extraClasses[$type]" ); - return $props; - } - - /** - * @return array - */ - private function getMenuProps() : array { - $contentNavigation = $this->buildContentNavigationUrls(); - $personalTools = self::getPersonalToolsForMakeListItem( - $this->buildPersonalUrls() - ); - $ptools = $this->getMenuData( 'personal', $personalTools ); - - return [ - 'data-personal-menu' => $ptools, - 'data-namespace-tabs' => $this->getMenuData( - 'namespaces', - $contentNavigation[ 'namespaces' ] ?? [], - self::MENU_TYPE_TABS - ), - 'data-variants' => $this->getMenuData( - 'variants', - $contentNavigation[ 'variants' ] ?? [], - self::MENU_TYPE_DROPDOWN, - true - ), - 'data-page-actions' => $this->getMenuData( - 'views', - $contentNavigation[ 'views' ] ?? [], - self::MENU_TYPE_TABS - ), - 'data-page-actions-more' => $this->getMenuData( - 'cactions', - $contentNavigation[ 'actions' ] ?? [], - self::MENU_TYPE_DROPDOWN - ), + return $portletData + [ + 'is-dropdown' => $type === self::MENU_TYPE_DROPDOWN, ]; } } diff --git a/includes/templates/Header.mustache b/includes/templates/Header.mustache index a17dc73..f1c2f42 100644 --- a/includes/templates/Header.mustache +++ b/includes/templates/Header.mustache @@ -11,5 +11,7 @@ {{>Logo}} {{#data-search-box}}{{>SearchBox}}{{/data-search-box}} - {{#data-personal-menu}}{{>Menu}}{{/data-personal-menu}} + {{#data-portlets}} + {{#data-personal}}{{>Menu}}{{/data-personal}} + {{/data-portlets}} diff --git a/includes/templates/Navigation.mustache b/includes/templates/Navigation.mustache index 4274a6e..8c8f27f 100644 --- a/includes/templates/Navigation.mustache +++ b/includes/templates/Navigation.mustache @@ -2,14 +2,16 @@

{{msg-navigation-heading}}

+ {{#data-portlets}}
- {{#data-namespace-tabs}}{{>Menu}}{{/data-namespace-tabs}} + {{#data-namespaces}}{{>Menu}}{{/data-namespaces}} {{#data-variants}}{{>Menu}}{{/data-variants}}
{{#data-page-actions}}{{>Menu}}{{/data-page-actions}} {{#data-page-actions-more}}{{>Menu}}{{/data-page-actions-more}}
+ {{/data-portlets}}
diff --git a/includes/templates/Sidebar.mustache b/includes/templates/Sidebar.mustache index bd80fe9..c9331bf 100644 --- a/includes/templates/Sidebar.mustache +++ b/includes/templates/Sidebar.mustache @@ -4,19 +4,19 @@ @prop string text string html-logo-attributes for site logo. Must be used inside tag e.g. `class="logo" lang="en-gb"` - MenuDefinition data-portals-first - MenuDefinition[] array-portals-rest + MenuDefinition data-portlets-first + MenuDefinition[] array-portlets-rest emphasized-sidebar-action data-emphasized-sidebar-action For displaying an emphasized action in the sidebar. }}
- {{#data-portals-first}}{{>Menu}}{{/data-portals-first}} + {{#data-portlets-first}}{{>Menu}}{{/data-portlets-first}} {{#data-emphasized-sidebar-action}} {{/data-emphasized-sidebar-action}} - {{#array-portals-rest}}{{>Menu}}{{/array-portals-rest}} - {{#data-portals-languages}}{{>Menu}}{{/data-portals-languages}} + {{#array-portlets-rest}}{{>Menu}}{{/array-portlets-rest}} + {{#data-portlets.data-languages}}{{>Menu}}{{/data-portlets.data-languages}}
diff --git a/includes/templates/legacy/Sidebar.mustache b/includes/templates/legacy/Sidebar.mustache index d5d414a..dfc4dbd 100644 --- a/includes/templates/legacy/Sidebar.mustache +++ b/includes/templates/legacy/Sidebar.mustache @@ -1,13 +1,13 @@ {{! See @typedef SidebarData - string html-logo-attributes for site logo. Must be used inside tag e.g. `class="logo" lang="en-gb"` }}
- {{#data-portals-first}}{{>Menu}}{{/data-portals-first}} - {{#array-portals-rest}}{{>Menu}}{{/array-portals-rest}} - {{#data-portals-languages}}{{>Menu}}{{/data-portals-languages}} + {{#data-portlets-first}}{{>Menu}}{{/data-portlets-first}} + {{#array-portlets-rest}}{{>Menu}}{{/array-portlets-rest}} + {{#data-portlets.data-languages}}{{>Menu}}{{/data-portlets.data-languages}}
diff --git a/includes/templates/skin-legacy.mustache b/includes/templates/skin-legacy.mustache index 4a23d2b..ed53cad 100644 --- a/includes/templates/skin-legacy.mustache +++ b/includes/templates/skin-legacy.mustache @@ -17,13 +17,13 @@ string html-after-content string msg-navigation-heading heading for entire navigation that is usually hidden to screen readers - MenuDefinition data-personal-menu - MenuDefinition data-namespace-tabs - MenuDefinition data-variants - MenuDefinition data-page-actions - MenuDefinition data-page-actions-more + MenuDefinition data-portlets.data-personal + MenuDefinition data-portlets.data-namespaces + MenuDefinition data-portlets.data-variants + MenuDefinition data-portlets.data-views + MenuDefinition data-portlets.data-actions object data-search-box. See SearchBox.mustache for documentation. - object data-sidebar. See Sidebar.mustache for documentation. + object data-portlets-sidebar. See Sidebar.mustache for documentation. object data-footer for footer template partial. see Footer.mustache for documentation. }}
@@ -53,17 +53,19 @@

{{msg-navigation-heading}}

- {{#data-personal-menu}}{{>Menu}}{{/data-personal-menu}} + {{#data-portlets}} + {{#data-personal}}{{>Menu}}{{/data-personal}}
- {{#data-namespace-tabs}}{{>Menu}}{{/data-namespace-tabs}} + {{#data-namespaces}}{{>Menu}}{{/data-namespaces}} {{#data-variants}}{{>Menu}}{{/data-variants}}
- {{#data-page-actions}}{{>Menu}}{{/data-page-actions}} - {{#data-page-actions-more}}{{>Menu}}{{/data-page-actions-more}} + {{#data-views}}{{>Menu}}{{/data-views}} + {{#data-actions}}{{>Menu}}{{/data-actions}} {{#data-search-box}}{{>SearchBox}}{{/data-search-box}}
+ {{/data-portlets}}
- {{#data-sidebar}}{{>legacy/Sidebar}}{{/data-sidebar}} + {{#data-portlets-sidebar}}{{>legacy/Sidebar}}{{/data-portlets-sidebar}}
{{#data-footer}}{{>Footer}}{{/data-footer}} diff --git a/includes/templates/skin.mustache b/includes/templates/skin.mustache index b755ae3..8c5a3ca 100644 --- a/includes/templates/skin.mustache +++ b/includes/templates/skin.mustache @@ -18,16 +18,17 @@ string html-after-content string msg-navigation-heading LogoOptions data-logos - MenuDefinition data-personal-menu - MenuDefinition data-namespace-tabs - MenuDefinition data-variants - MenuDefinition data-page-actions - MenuDefinition data-page-actions-more + object data-portlets + MenuDefinition data-portlets.data-personal + MenuDefinition data-portlets.data-namespaces + MenuDefinition data-portlets.data-variants + MenuDefinition data-portlets.data-views + MenuDefinition data-portlets.data-actions object data-search-box. See SearchBox.mustache for documentation. boolean sidebar-visible For users that want to see the sidebar on initial render, this should be true. string msg-vector-action-toggle-sidebar The label used by the sidebar button. - object data-sidebar. See Sidebar.mustache for documentation. + object data-portlets-sidebar. See Sidebar.mustache for documentation. object data-footer for footer template partial. see Footer.mustache for documentation. }}
@@ -43,7 +44,7 @@ {{>Header}}
- {{#data-sidebar}}{{>Sidebar}}{{/data-sidebar}} + {{#data-portlets-sidebar}}{{>Sidebar}}{{/data-portlets-sidebar}} {{>Navigation}}
{{! `role` is unnecessary but kept to support selectors in any gadgets or user styles. }} diff --git a/resources/skins.vector.styles/legacy/Sidebar.less b/resources/skins.vector.styles/legacy/Sidebar.less index 702b3ff..ac15e4f 100644 --- a/resources/skins.vector.styles/legacy/Sidebar.less +++ b/resources/skins.vector.styles/legacy/Sidebar.less @@ -4,7 +4,7 @@ #mw-panel { font-size: @font-size-nav-main; - .portal-first { + nav:first-child { background-image: none; h3 { diff --git a/resources/skins.vector.styles/legacy/layout.less b/resources/skins.vector.styles/legacy/layout.less index c4c4ea1..6d49caf 100644 --- a/resources/skins.vector.styles/legacy/layout.less +++ b/resources/skins.vector.styles/legacy/layout.less @@ -146,6 +146,11 @@ body { left: 0; } +// hide the heading of the first menu +#p-logo + .mw-portlet h3 { + display: none; +} + .mw-footer { margin-left: 10em; margin-top: 0; diff --git a/skin.json b/skin.json index 9b55fa9..4d75743 100644 --- a/skin.json +++ b/skin.json @@ -29,6 +29,7 @@ "mediawiki.ui.icon" ], "messages": [ + "tooltip-p-logo", "vector-opt-out-tooltip", "vector-opt-out", "navigation-heading", diff --git a/stories/MenuPortal.stories.data.js b/stories/MenuPortal.stories.data.js index c3b5d9c..68e6c55 100644 --- a/stories/MenuPortal.stories.data.js +++ b/stories/MenuPortal.stories.data.js @@ -50,7 +50,7 @@ export const PORTALS = { }, navigation: { id: 'p-navigation', - class: 'vector-menu-portal portal portal-first', + class: 'vector-menu-portal portal', 'html-tooltip': 'A message tooltip-p-navigation must exist for this to appear', label: 'Navigation', 'html-user-language-attributes': htmlUserLanguageAttributes, diff --git a/stories/Sidebar.stories.data.js b/stories/Sidebar.stories.data.js index 60d58a0..4efc214 100644 --- a/stories/Sidebar.stories.data.js +++ b/stories/Sidebar.stories.data.js @@ -1,11 +1,8 @@ -/* eslint-disable quotes */ - import sidebarTemplate from '!!raw-loader!../includes/templates/Sidebar.mustache'; import sidebarLegacyTemplate from '!!raw-loader!../includes/templates/legacy/Sidebar.mustache'; import { vectorMenuTemplate } from './MenuDropdown.stories.data'; import { PORTALS } from './MenuPortal.stories.data'; -const HTML_LOGO_ATTRIBUTES = `class="mw-wiki-logo" href="/wiki/Main_Page" title="Visit the main page"`; const SIDEBAR_BEFORE_OUTPUT_HOOKINFO = `Beware: Portals can be added, removed or reordered using SidebarBeforeOutput hook as in this example.`; @@ -25,34 +22,31 @@ export const OPT_OUT_DATA = { export const SIDEBAR_DATA = { withNoPortals: { - 'array-portals-rest': [], - 'html-logo-attributes': HTML_LOGO_ATTRIBUTES + 'array-portlets-rest': [] }, withPortals: { - 'data-portals-first': PORTALS.navigation, - 'array-portals-rest': [ + 'data-portlets-first': PORTALS.navigation, + 'array-portlets-rest': [ PORTALS.toolbox, PORTALS.otherProjects ], - 'data-portals-languages': PORTALS.langlinks, - 'html-logo-attributes': HTML_LOGO_ATTRIBUTES + 'data-portals-languages': PORTALS.langlinks }, withoutLogo: { 'data-portals-languages': PORTALS.langlinks, 'array-portals-first': PORTALS.navigation, - 'array-portals-rest': [ + 'array-portlets-rest': [ PORTALS.toolbox, PORTALS.otherProjects ] }, thirdParty: { - 'array-portals-rest': [ + 'array-portlets-rest': [ PORTALS.toolbox, PORTALS.navigation, { 'html-portal-content': SIDEBAR_BEFORE_OUTPUT_HOOKINFO } - ], - 'html-logo-attributes': HTML_LOGO_ATTRIBUTES + ] } }; diff --git a/stories/skin.stories.data.js b/stories/skin.stories.data.js index eb5d055..7a9767c 100644 --- a/stories/skin.stories.data.js +++ b/stories/skin.stories.data.js @@ -19,32 +19,38 @@ import { logoTemplate } from './Logo.stories.data'; export const NAVIGATION_TEMPLATE_DATA = { loggedInWithVariantsAndOptOut: Object.assign( {}, { - 'data-personal-menu': PERSONAL_MENU_TEMPLATE_DATA.loggedInWithEcho, - 'data-namespace-tabs': namespaceTabsData, - 'data-page-actions': pageActionsData, - 'data-variants': variantsData, + 'data-portlets': { + 'data-personal': PERSONAL_MENU_TEMPLATE_DATA.loggedInWithEcho, + 'data-namespaces': namespaceTabsData, + 'data-views': pageActionsData, + 'data-variants': variantsData + }, 'data-search-box': searchBoxData, - 'data-sidebar': SIDEBAR_DATA.withPortals, + 'data-portlets-sidebar': SIDEBAR_DATA.withPortals, 'msg-navigation-heading': 'Navigation menu', 'html-logo-attributes': `class="mw-wiki-logo" href="/wiki/Main_Page" title="Visit the main page"` }, OPT_OUT_DATA ), loggedOutWithVariants: { - 'data-personal-menu': PERSONAL_MENU_TEMPLATE_DATA.loggedOut, - 'data-namespace-tabs': namespaceTabsData, - 'data-page-actions': pageActionsData, - 'data-variants': variantsData, + 'data-portlets': { + 'data-personal': PERSONAL_MENU_TEMPLATE_DATA.loggedOut, + 'data-namespaces': namespaceTabsData, + 'data-views': pageActionsData, + 'data-variants': variantsData + }, 'data-search-box': searchBoxData, - 'data-sidebar': SIDEBAR_DATA.withPortals, + 'data-portlets-sidebar': SIDEBAR_DATA.withPortals, 'msg-navigation-heading': 'Navigation menu', 'html-logo-attributes': `class="mw-wiki-logo" href="/wiki/Main_Page" title="Visit the main page"` }, loggedInWithMoreActions: { - 'data-personal-menu': PERSONAL_MENU_TEMPLATE_DATA.loggedInWithEcho, - 'data-namespace-tabs': namespaceTabsData, - 'data-page-actions': pageActionsData, - 'data-page-actions-more': moreData, + 'data-portlets': { + 'data-personal': PERSONAL_MENU_TEMPLATE_DATA.loggedInWithEcho, + 'data-namespaces': namespaceTabsData, + 'data-views': pageActionsData, + 'data-actions': moreData + }, 'data-search-box': searchBoxData, - 'data-sidebar': SIDEBAR_DATA.withPortals, + 'data-portlets-sidebar': SIDEBAR_DATA.withPortals, 'msg-navigation-heading': 'Navigation menu', 'html-logo-attributes': `class="mw-wiki-logo" href="/wiki/Main_Page" title="Visit the main page"` } diff --git a/stories/types.js b/stories/types.js index 8e03958..9bae848 100644 --- a/stories/types.js +++ b/stories/types.js @@ -32,8 +32,8 @@ /** * @typedef {Object} SidebarData * @property {MenuDefinition} data-portals-languages - * @property {MenuDefinition} data-portals-first - * @property {MenuDefinition[]} array-portals-rest + * @property {MenuDefinition} data-portlets-first + * @property {MenuDefinition[]} array-portlets-rest */ /** diff --git a/tests/phpunit/integration/SkinVectorTest.php b/tests/phpunit/integration/SkinVectorTest.php index 35143bd..1523ea9 100644 --- a/tests/phpunit/integration/SkinVectorTest.php +++ b/tests/phpunit/integration/SkinVectorTest.php @@ -45,9 +45,9 @@ class SkinVectorTest extends MediaWikiIntegrationTestCase { } /** - * @covers ::getMenuProps + * @covers ::getTemplateData */ - public function testGetMenuProps() { + public function testGetTemplateData() { $title = Title::newFromText( 'SkinVector' ); $context = RequestContext::getMain(); $context->setTitle( $title ); @@ -78,9 +78,9 @@ class SkinVectorTest extends MediaWikiIntegrationTestCase { ] ); $openVectorTemplate = TestingAccessWrapper::newFromObject( $vectorTemplate ); - $props = $openVectorTemplate->getMenuProps(); - $views = $props['data-page-actions']; - $namespaces = $props['data-namespace-tabs']; + $props = $openVectorTemplate->getTemplateData()['data-portlets']; + $views = $props['data-views']; + $namespaces = $props['data-namespaces']; $this->assertSame( [ @@ -91,14 +91,13 @@ class SkinVectorTest extends MediaWikiIntegrationTestCase { 'html-items' => '', 'html-after-portal' => '', 'label' => $context->msg( 'views' )->text(), - 'label-id' => 'p-views-label', 'is-dropdown' => false, ], $views ); $variants = $props['data-variants']; - $actions = $props['data-page-actions-more']; + $actions = $props['data-actions']; $this->assertSame( 'mw-portlet mw-portlet-namespaces vector-menu vector-menu-tabs', $namespaces['class'] @@ -113,7 +112,7 @@ class SkinVectorTest extends MediaWikiIntegrationTestCase { ); $this->assertSame( 'mw-portlet mw-portlet-personal vector-menu', - $props['data-personal-menu']['class'] + $props['data-personal']['class'] ); }