From ec382a8c86b50af7f7a473c7a4eecf3a2153571c Mon Sep 17 00:00:00 2001 From: Nicholas Ray Date: Tue, 17 Mar 2020 14:21:33 -0600 Subject: [PATCH] Add opt-out link to Sidebar for Vector/Logged-in Users Without Abstractions This commit is singularly focused on adding a link to the sidebar for Vector, logged-in users. It does the bare minimum to fulfill the requirements of T243281. Additionally, it will help to answer the question "Do we need to use abstractions (other than maybe different templates) to separate Legacy Vector from Vector" by intentionally leaving out any abstractions in order to make it easier to compare with a follow-up patch (Ib2ef15180df73360cc1de25b893e49d415d23e1a) which does use abstractions. It is a good thing to question whether or not we need addtional abstractions in VectorTemplate and if they will help us as unnecessary abstractions can have the opposite effect and just lead to further frustrations down the road. Therefore, I urge you, the reviewer, to let me know your thoughts! If abstractions are viewed as not making our lives any easier, the follow-up patches may be completely discarded and that's totally okay with me. :) I think it's a good think to talk about now though. Important changes: * The VectorTemplate constructor was changed to allow injecting the config, templateParser, and isLegacy boolean (only the config was allowed before this commit). According to MediaWiki's Stable Interface Policy, "Constructor signatures are generally considered unstable unless explicitly declared stable for calling" [3]. Given that VecorTemplate's constructor is not marked as stable, it is justified to do this without warning according to the policy. * Due to the above, the 'setTemplate' method is no longer needed and was marked as deprecated. * VectorTemplateTest was made to adapt to the new VectorTemplate constructor. Additionally, it now extends from MediaWikiIntegrationTestCase which my intelliphense server can pick up. I *think* MediaWikiTestCase is just an alias to MediaWikiIntegrationTestCase [1] and MediaWikiTestCase file was renamed to MediaWikiIntegrationTestCase in [2], but I'm willing to change it back if there is pushback to this. Open questions: * What are VectorTemplate's responsibilities? To me, it acts right now as a controller (because it echos the full HTML string from the template), a model (because SkinTemplate::prepareQuickTemplate sets data on it which it later retrieves through `$this->get()`), a presenter (because it adds data tailored for a web-centric view), and a view (because it renders HTML strings instead of letting the view/template be solely responsible for that). Arguably, some business logic might be mixed in there as well (because it checks to see if a User is logged in/has necessary permissions to show x which my changes here add to). This might not be a problem if we keep VectorTemplate relatively small, but will it remain this way as we progress further in Desktop Improvements? * How do we write tests for VectorTemplate without exposing unnecessary public methods? For example, if I want to test the `getSkinData()` method to see what state will be sent to the template, how should I do this? One option might be to use `TestingAccessWrapper` to expose these private methods which is what `VectorTemplateTest::testbuildViewsProps()` does. Another option is to accept this method as public. Is there a better way? Keep in mind that even with access to this method, there might be many things to mock. [1] https://github.com/wikimedia/mediawiki/blob/0030cb525be6cabc1d63de80586b2017d4bbe354/tests/common/TestsAutoLoader.php#L64 [2] Ie717b0ecf4fcfd089d46248f14853c80b7ef4a76 [3] https://www.mediawiki.org/wiki/Stable_interface_policy Bug: T243281 Change-Id: I0571b041bcd7f19bec9f103fa7bccdd093f6394d --- i18n/en.json | 1 + i18n/qqq.json | 1 + includes/SkinVector.php | 21 +++++-- includes/VectorTemplate.php | 63 ++++++++++++++++--- includes/templates/Sidebar.mustache | 10 +++ .../EmphasizedSidebarAction.less | 12 ++++ resources/skins.vector.styles/Portal.less | 8 +-- resources/skins.vector.styles/Sidebar.less | 2 +- resources/skins.vector.styles/index.less | 1 + stories/Navigation.stories.data.js | 10 +++ stories/Sidebar.stories.data.js | 13 ++++ stories/skin.stories.js | 23 ++++++- .../integration/VectorTemplateTest.php | 15 +++-- variables.less | 13 +++- 14 files changed, 167 insertions(+), 26 deletions(-) create mode 100644 resources/skins.vector.styles/EmphasizedSidebarAction.less diff --git a/i18n/en.json b/i18n/en.json index feef622f..2488b915 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -6,6 +6,7 @@ "vector-skin-desc": "Modern version of MonoBook with fresh look and many usability improvements", "prefs-vector-enable-vector-1-label": "Use Legacy Vector", "prefs-vector-enable-vector-1-help": "Over the next few years, we will be gradually updating the Vector skin. Legacy Vector will allow you to view the old version of Vector (as of December 2019). To learn more about the updates, go to our [[mw:Reading/Web/Desktop_Improvements|project page]].", + "vector-opt-out": "Switch to old look", "vector.css": "/* All CSS here will be loaded for users of the Vector skin */", "vector.js": "/* All JavaScript here will be loaded for users of the Vector skin */", "vector-action-addsection": "Add topic", diff --git a/i18n/qqq.json b/i18n/qqq.json index d4e2dd93..442e0343 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -17,6 +17,7 @@ "vector-skin-desc": "{{desc|what=skin|name=Vector|url=https://www.mediawiki.org/wiki/Skin:Vector}}", "prefs-vector-enable-vector-1-label": "Label for the checkbox to force Legacy Vector operation accessible via Special:Preferences. When this checkbox is enabled, the December 2019 of Vector is used. When this checkbox is disabled, the actively developed version of Vector is used instead.", "prefs-vector-enable-vector-1-help": "Detail explaining the operation of the prefs-vector-enable-vector-1-label checkbox.", + "vector-opt-out": "Text of link that takes the user to the Special:Preferences page so they can opt-out of the latest version of Vector and go back to legacy Vector.", "vector.css": "{{optional}}", "vector.js": "{{optional}}", "vector-action-addsection": "Used in the Vector skin. See for example {{canonicalurl:Talk:Main_Page|useskin=vector}}\n{{Identical|Add topic}}", diff --git a/includes/SkinVector.php b/includes/SkinVector.php index ba2be574..29a00102 100644 --- a/includes/SkinVector.php +++ b/includes/SkinVector.php @@ -67,8 +67,7 @@ class SkinVector extends SkinTemplate { public function getDefaultModules() { $modules = parent::getDefaultModules(); // add vector skin styles and vector module - $module = $this->getUser()->getOption( Constants::PREF_KEY_SKIN_VERSION ) - === Constants::SKIN_VERSION_LEGACY ? 'skins.vector.styles.legacy' : 'skins.vector.styles'; + $module = $this->isLegacy() ? 'skins.vector.styles.legacy' : 'skins.vector.styles'; $modules['styles']['skin'][] = $module; $modules['core'][] = 'skins.vector.js'; @@ -85,9 +84,10 @@ class SkinVector extends SkinTemplate { * @return VectorTemplate */ protected function setupTemplate( $classname ) { - $template = new VectorTemplate( $this->getConfig() ); - $template->setTemplateParser( new TemplateParser( __DIR__ . '/templates' ) ); - return $template; + $tp = new TemplateParser( __DIR__ . '/templates' ); + $vectorTemplate = new VectorTemplate( $this->getConfig(), $tp, $this->isLegacy() ); + + return $vectorTemplate; } /** @@ -98,4 +98,15 @@ class SkinVector extends SkinTemplate { public function shouldPreloadLogo() { return true; } + + /** + * Whether or not the legacy skin is being used. + * + * @return bool + */ + private function isLegacy() { + // Note: This will be replaced with FeatureManager when it is ready. + return $this->getUser()->getOption( Constants::PREF_KEY_SKIN_VERSION ) + === Constants::SKIN_VERSION_LEGACY; + } } diff --git a/includes/VectorTemplate.php b/includes/VectorTemplate.php index e46d7e02..ca386056 100644 --- a/includes/VectorTemplate.php +++ b/includes/VectorTemplate.php @@ -29,15 +29,38 @@ use MediaWiki\MediaWikiServices; * @ingroup Skins */ class VectorTemplate extends BaseTemplate { + /** + * T243281: Code used to track clicks to opt-out link. + * + * The "vct" substring is used to describe the newest "Vector" (non-legacy) + * feature. The "w" describes the web platform. The "1" describes the version + * of the feature. + * + * @see https://wikitech.wikimedia.org/wiki/Provenance + * @var string + */ + private const OPT_OUT_LINK_TRACKING_CODE = 'vctw1'; /** @var TemplateParser */ private $templateParser; + /** @var bool */ + private $isLegacy; + /** - * @param TemplateParser $parser + * @param Config $config + * @param TemplateParser $templateParser + * @param bool $isLegacy */ - public function setTemplateParser( TemplateParser $parser ) { - $this->templateParser = $parser; + public function __construct( + Config $config, + TemplateParser $templateParser, + bool $isLegacy + ) { + parent::__construct( $config ); + + $this->templateParser = $templateParser; + $this->isLegacy = $isLegacy; } /** @@ -55,9 +78,10 @@ class VectorTemplate extends BaseTemplate { } /** - * Outputs the entire contents of the HTML page + * @return array Returns an array of data shared between Vector and legacy + * Vector. */ - public function execute() { + private function getSkinData() : array { $contentNavigation = $this->get( 'content_navigation', [] ); $this->set( 'namespace_urls', $contentNavigation[ 'namespaces' ] ); $this->set( 'view_urls', $contentNavigation[ 'views' ] ); @@ -92,7 +116,6 @@ class VectorTemplate extends BaseTemplate { $htmlHookVectorBeforeFooter = ob_get_contents(); ob_end_clean(); - $tp = $this->getTemplateParser(); // Naming conventions for Mustache parameters. // // Value type (first segment): @@ -109,7 +132,7 @@ class VectorTemplate extends BaseTemplate { // It should be followed by the name of the hook in hyphenated lowercase. // // Conditionally used values must use null to indicate absence (not false or ''). - $params = [ + $commonSkinData = [ 'html-headelement' => $this->get( 'headelement', '' ), 'html-sitenotice' => $this->get( 'sitenotice', null ), 'html-indicators' => $this->getIndicators(), @@ -171,8 +194,30 @@ class VectorTemplate extends BaseTemplate { ], ]; - // Prepare and output the HTML response - echo $tp->processTemplate( 'index', $params ); + // The following logic is unqiue to Vector (not used by legacy Vector) and + // is planned to be moved in a follow-up patch. + if ( !$this->isLegacy && $this->getSkin()->getUser()->isLoggedIn() ) { + $commonSkinData['data-navigation']['data-sidebar']['data-emphasized-sidebar-action'] = [ + 'href' => SpecialPage::getTitleFor( + 'Preferences', + false, + // FIXME: should be mw-prefsection-rendering-skin-skin-prefs but this doesn't currently work + // possibly due to the issues T246491 + 'mw-prefsection-rendering' + )->getLinkURL( 'wprov=' . self::OPT_OUT_LINK_TRACKING_CODE ), + 'text' => $this->getMsg( 'vector-opt-out' )->text() + ]; + } + + return $commonSkinData; + } + + /** + * Renders the entire contents of the HTML page. + */ + public function execute() { + $tp = $this->getTemplateParser(); + echo $tp->processTemplate( 'index', $this->getSkinData() ); } /** diff --git a/includes/templates/Sidebar.mustache b/includes/templates/Sidebar.mustache index 82dce0b7..c4a3b69c 100644 --- a/includes/templates/Sidebar.mustache +++ b/includes/templates/Sidebar.mustache @@ -1,6 +1,11 @@ {{! + @typedef object emphasized-sidebar-action + @prop string href + @prop string text + string html-logo-attributes for site logo. Must be used inside tag e.g. `class="logo" lang="en-gb"` array array-portals contains options for Portal template + emphasized-sidebar-action data-emphasized-sidebar-action For displaying an emphasized action in the sidebar. }}
@@ -10,6 +15,11 @@ {{#array-portals-first}} {{>Portal}} {{/array-portals-first}} + {{#data-emphasized-sidebar-action}} +
+ {{text}} +
+ {{/data-emphasized-sidebar-action}} {{#array-portals-rest}} {{>Portal}} {{/array-portals-rest}} diff --git a/resources/skins.vector.styles/EmphasizedSidebarAction.less b/resources/skins.vector.styles/EmphasizedSidebarAction.less new file mode 100644 index 00000000..6efca5ee --- /dev/null +++ b/resources/skins.vector.styles/EmphasizedSidebarAction.less @@ -0,0 +1,12 @@ +@import '../../variables.less'; + +.vector-emphasized-sidebar-action { + // Align with the portal heading/links + // `.portal` + `.portal .body` + margin: 8px 0 8px @margin-start-portal + @margin-start-portal-body; +} + +.vector-emphasized-sidebar-action-link { + font-size: @font-size-portal-list-item; + font-weight: bold; +} diff --git a/resources/skins.vector.styles/Portal.less b/resources/skins.vector.styles/Portal.less index b26e06c2..3fe8591b 100644 --- a/resources/skins.vector.styles/Portal.less +++ b/resources/skins.vector.styles/Portal.less @@ -2,14 +2,14 @@ @import 'mediawiki.mixins.less'; .portal { - margin: 0 0.6em 0 0.7em; + margin: 0 0.6em 0 @margin-start-portal; padding: 0.25em 0; direction: ltr; h3 { color: @color-nav-subtle; font-weight: normal; - margin: 0.5em 0 0 ( @margin-left-nav-main-body / @font-size-nav-main-heading ); + margin: 0.5em 0 0 ( @margin-start-nav-main-body / @font-size-nav-main-heading ); padding: 0.25em 0; cursor: default; border: 0; @@ -21,7 +21,7 @@ background-image: linear-gradient( to right, transparent 0, #c8ccd1 35%, #c8ccd1 70%, transparent 100% ); // Standard (Firefox 16+, IE 10+, Safari 6.1+, Chrome 26+) background-repeat: no-repeat; background-size: 100% @border-width-base; - margin-left: @margin-left-nav-main-body; + margin-left: @margin-start-portal-body; padding-top: 0; ul { @@ -33,7 +33,7 @@ li { margin: 0; padding: 0.25em 0; - font-size: @font-size-nav-main-body; + font-size: @font-size-portal-list-item; line-height: @line-height-nav; word-wrap: break-word; diff --git a/resources/skins.vector.styles/Sidebar.less b/resources/skins.vector.styles/Sidebar.less index 0cc3ccc3..ec5f07d0 100644 --- a/resources/skins.vector.styles/Sidebar.less +++ b/resources/skins.vector.styles/Sidebar.less @@ -34,7 +34,7 @@ .body { background-image: none; - margin-left: @margin-left-nav-main-body; + margin-left: @margin-start-nav-main-body; } } } diff --git a/resources/skins.vector.styles/index.less b/resources/skins.vector.styles/index.less index 45c017e1..ba865dca 100644 --- a/resources/skins.vector.styles/index.less +++ b/resources/skins.vector.styles/index.less @@ -12,6 +12,7 @@ @import 'Portal.less'; @import 'Sidebar.less'; @import 'Footer.less'; + @import 'EmphasizedSidebarAction.less'; @import 'externalLinks.less'; } diff --git a/stories/Navigation.stories.data.js b/stories/Navigation.stories.data.js index b5c43852..473279fe 100644 --- a/stories/Navigation.stories.data.js +++ b/stories/Navigation.stories.data.js @@ -14,6 +14,16 @@ export const NAVIGATION_TEMPLATE_PARTIALS = Object.assign( {}, SIDEBAR_TEMPLATE_ } ); export const NAVIGATION_TEMPLATE_DATA = { + loggedInWithVariantsAndOptOut: { + 'data-personal-menu': PERSONAL_MENU_TEMPLATE_DATA.loggedOut, + 'data-namespace-tabs': namespaceTabsData, + 'data-page-actions': pageActionsData, + 'data-variants': variantsData, + 'data-search-box': searchBoxData, + 'data-sidebar': SIDEBAR_DATA.withPortalsAndOptOut, + 'html-navigation-heading': 'Navigation menu', + 'html-logo-attributes': `class="mw-wiki-logo" href="/wiki/Main_Page" title="Visit the main page"` + }, loggedOutWithVariants: { 'data-personal-menu': PERSONAL_MENU_TEMPLATE_DATA.loggedOut, 'data-namespace-tabs': namespaceTabsData, diff --git a/stories/Sidebar.stories.data.js b/stories/Sidebar.stories.data.js index 1bfcf91f..27814d5d 100644 --- a/stories/Sidebar.stories.data.js +++ b/stories/Sidebar.stories.data.js @@ -17,6 +17,19 @@ export const SIDEBAR_DATA = { 'array-portals-rest': [], 'html-logo-attributes': HTML_LOGO_ATTRIBUTES }, + withPortalsAndOptOut: { + 'array-portals-first': PORTALS.navigation, + 'data-emphasized-sidebar-action': { + href: '#', + text: 'Switch to old look' + }, + 'array-portals-rest': [ + PORTALS.toolbox, + PORTALS.otherProjects, + PORTALS.langlinks + ], + 'html-logo-attributes': HTML_LOGO_ATTRIBUTES + }, withPortals: { 'array-portals-first': PORTALS.navigation, 'array-portals-rest': [ diff --git a/stories/skin.stories.js b/stories/skin.stories.js index 04e7797e..3945d453 100644 --- a/stories/skin.stories.js +++ b/stories/skin.stories.js @@ -35,7 +35,7 @@ const HTML_INDICATORS = `
`; -export const vector2019LoggedOut = () => mustache.render( skinTemplate, { +export const vectorLegacyLoggedOut = () => mustache.render( skinTemplate, { 'html-title': 'Vector 2019', 'page-isarticle': true, 'msg-tagline': 'From Wikipedia, the free encyclopedia', @@ -60,7 +60,7 @@ export const vector2019LoggedOut = () => mustache.render( skinTemplate, { 'html-subtitle': placeholder( 'Extensions can configure subtitle', 20 ) }, TEMPLATE_PARTIALS ); -export const vector2019LoggedIn = () => mustache.render( skinTemplate, { +export const vectorLegacyLoggedIn = () => mustache.render( skinTemplate, { 'html-title': 'Vector 2019', 'page-isarticle': true, 'msg-tagline': 'From Wikipedia, the free encyclopedia', @@ -78,3 +78,22 @@ export const vector2019LoggedIn = () => mustache.render( skinTemplate, { 'html-printfooter': `Retrieved from ‘https://en.wikipedia.org/w/index.php?title=this&oldid=blah’`, 'html-catlinks': placeholder( 'Category links component from mediawiki core', 50 ) }, TEMPLATE_PARTIALS ); + +export const vectorLoggedIn = () => mustache.render( skinTemplate, { + 'html-title': 'Vector 2020', + 'page-isarticle': true, + 'msg-tagline': 'From Wikipedia, the free encyclopedia', + 'html-userlangattributes': htmluserlangattributes, + 'msg-jumptonavigation': 'Jump to navigation', + 'msg-jumptosearch': 'Jump to search', + 'data-navigation': NAVIGATION_TEMPLATE_DATA.loggedInWithVariantsAndOptOut, + + // site specific + 'data-footer': FOOTER_TEMPLATE_DATA, + 'html-sitenotice': placeholder( 'a site notice or central notice banner may go here', 70 ), + + // article dependent + 'html-bodycontent': placeholder( 'Article content goes here' ), + 'html-printfooter': `Retrieved from ‘https://en.wikipedia.org/w/index.php?title=this&oldid=blah’`, + 'html-catlinks': placeholder( 'Category links component from mediawiki core', 50 ) +}, TEMPLATE_PARTIALS ); diff --git a/tests/phpunit/integration/VectorTemplateTest.php b/tests/phpunit/integration/VectorTemplateTest.php index 0706b023..19d63389 100644 --- a/tests/phpunit/integration/VectorTemplateTest.php +++ b/tests/phpunit/integration/VectorTemplateTest.php @@ -1,6 +1,10 @@ setTemplateParser( new \TemplateParser() ); + $template = new VectorTemplate( + GlobalVarConfig::newInstance(), + new TemplateParser(), + true + ); return $template; } @@ -128,7 +135,7 @@ class VectorTemplateTest extends \MediaWikiTestCase { */ public function testbuildViewsProps() { $langAttrs = 'LANG_ATTRIBUTES'; - $vectorTemplate = new \VectorTemplate( \GlobalVarConfig::newInstance() ); + $vectorTemplate = $this->provideVectorTemplateObject(); $vectorTemplate->set( 'view_urls', [] ); $vectorTemplate->set( 'skin', new \SkinVector() ); $vectorTemplate->set( 'userlangattributes', $langAttrs ); diff --git a/variables.less b/variables.less index 0e4a93c7..eb8afa11 100644 --- a/variables.less +++ b/variables.less @@ -74,7 +74,18 @@ @font-size-nav-main-heading: unit( 12 / @font-size-browser, em ); // Equals `0.75em`. @font-size-nav-main-body: unit( 12 / @font-size-browser, em ); -@margin-left-nav-main-body: 0.5em; +@margin-start-nav-main-body: 0.5em; + +// Navigation: Portal +// Font size of the Portal links. +@font-size-portal-list-item: @font-size-nav-main-body; + +// Margin space from the start of the Portal (left edge in LTR +// languages). +@margin-start-portal: 0.7em; + +// Margin space from the start of the Portal body (left edge in LTR languages). +@margin-start-portal-body: @margin-start-nav-main-body; // Navigation: Personal tools @background-position-nav-personal-icon: left ( 4 / @font-size-browser / @font-size-nav-personal );