From b93b7eda7c1f0c82c95307fc675c65ce110b4bf3 Mon Sep 17 00:00:00 2001 From: jdlrobson Date: Thu, 28 Sep 2017 15:06:39 -0500 Subject: [PATCH] Render add discussion button in PHP not JS The talk page JavaScript progressively enhances an existing button in the page. Remove the frontend logic and rely entirely on whether the button is in the page or not. Additional change: * The browser tests incorrectly suggest a user needs 5 edits to be able to use the talk feature. This is not true. They just need to be logged in. Update that logic. Bug: T167728 Change-Id: Iacedea30bdd0775b3d785db5b143abafd7a18b39 --- includes/skins/SkinMinerva.php | 29 ++++++++++++++++----- resources/skins.minerva.talk/init.js | 39 ++++++++++++---------------- skin.json | 3 --- tests/browser/features/talk.feature | 26 ++++++++++--------- 4 files changed, 53 insertions(+), 44 deletions(-) diff --git a/includes/skins/SkinMinerva.php b/includes/skins/SkinMinerva.php index 22459c1..10fa820 100644 --- a/includes/skins/SkinMinerva.php +++ b/includes/skins/SkinMinerva.php @@ -991,14 +991,24 @@ class SkinMinerva extends SkinTemplate implements ICustomizableSkin { * Returns an array with details for a talk button. * @param Title $talkTitle Title object of the talk page * @param array $talkButton Array with data of desktop talk button + * @param bool $addSection (optional) when added the talk button will render + * as an add topic button. Defaults to false. * @return array */ - protected function getTalkButton( $talkTitle, $talkButton ) { + protected function getTalkButton( $talkTitle, $talkButton, $addSection = false ) { + if ( $addSection ) { + $params = [ 'action' => 'edit', 'section' => 'new' ]; + $className = 'talk continue add'; + } else { + $params = []; + $className = 'talk'; + } + return [ 'attributes' => [ - 'href' => $talkTitle->getLinkURL(), + 'href' => $talkTitle->getLinkURL( $params ), 'data-title' => $talkTitle->getFullText(), - 'class' => 'talk', + 'class' => $className, ], 'label' => $talkButton['text'], ]; @@ -1037,10 +1047,16 @@ class SkinMinerva extends SkinTemplate implements ICustomizableSkin { // FIXME [core]: This seems unnecessary.. $subjectId = $title->getNamespaceKey( '' ); $talkId = $subjectId === 'main' ? 'talk' : "{$subjectId}_talk"; - if ( isset( $namespaces[$talkId] ) && !$title->isTalkPage() ) { + + if ( isset( $namespaces[$talkId] ) ) { $talkButton = $namespaces[$talkId]; $talkTitle = $title->getTalkPage(); - $buttons['talk'] = $this->getTalkButton( $talkTitle, $talkButton ); + if ( $title->isTalkPage() ) { + $talkButton['text'] = wfMessage( 'minerva-talk-add-topic' ); + $buttons['talk'] = $this->getTalkButton( $title, $talkButton, true ); + } else { + $buttons['talk'] = $this->getTalkButton( $talkTitle, $talkButton ); + } } } @@ -1265,8 +1281,7 @@ class SkinMinerva extends SkinTemplate implements ICustomizableSkin { protected function isTalkAllowed() { $title = $this->getTitle(); return $this->isAllowedPageAction( 'talk' ) && - !$title->isTalkPage() && - $title->canHaveTalkPage() && + ( $title->isTalkPage() || $title->canHaveTalkPage() ) && $this->getUser()->isLoggedIn(); } diff --git a/resources/skins.minerva.talk/init.js b/resources/skins.minerva.talk/init.js index 7d8c1d7..c55846b 100644 --- a/resources/skins.minerva.talk/init.js +++ b/resources/skins.minerva.talk/init.js @@ -1,15 +1,13 @@ ( function ( M, $ ) { var loader = M.require( 'mobile.startup/rlModuleLoader' ), LoadingOverlay = M.require( 'mobile.startup/LoadingOverlay' ), - user = M.require( 'mobile.startup/user' ), - Button = M.require( 'mobile.startup/Button' ), $talk = $( '.talk' ), // use the plain return value here - T128273 title = $talk.attr( 'data-title' ), - page = M.getCurrentPage(), overlayManager = M.require( 'skins.minerva.scripts/overlayManager' ), skin = M.require( 'skins.minerva.scripts/skin' ), - pageTitle, talkTitle; + inTalkNamespace = false, + pageTitle, talkTitle, talkNs, pageNs; // if there's no title for any reason, don't do anything if ( !title ) { @@ -25,12 +23,15 @@ // The method to get associated namespaces will change later (maybe), see T487 pageTitle = mw.Title.newFromText( mw.config.get( 'wgPageName' ) ); talkTitle = mw.Title.newFromText( title ); - if ( - !pageTitle || - !talkTitle || - ( pageTitle.getMainText() !== talkTitle.getMainText() ) || - ( pageTitle.getNamespaceId() + 1 !== talkTitle.getNamespaceId() ) - ) { + + if ( !pageTitle || !talkTitle || pageTitle.getMainText() !== talkTitle.getMainText() ) { + return; + } + talkNs = talkTitle.getNamespaceId(); + pageNs = pageTitle.getNamespaceId(); + inTalkNamespace = talkNs === pageNs; + + if ( pageNs + 1 !== talkNs && !inTalkNamespace ) { return; } @@ -63,24 +64,18 @@ */ function init() { $talk.on( 'click', function () { - window.location.hash = '#/talk'; + if ( $talk.hasClass( 'add' ) ) { + window.location.hash = '#/talk/new'; + } else { + window.location.hash = '#/talk'; + } return false; } ); } init(); - // add an "add discussion" button to talk pages (only for logged in users) - if ( - !user.isAnon() && - ( page.inNamespace( 'talk' ) || page.inNamespace( 'user_talk' ) ) - ) { - new Button( { - label: mw.msg( 'minerva-talk-add-topic' ), - href: '#/talk/new', - progressive: true - } ).prependTo( '#content #bodyContent' ); - + if ( inTalkNamespace ) { // reload the page after the new discussion was added M.on( 'talk-added-wo-overlay', function () { var loadingOverlay = new LoadingOverlay(); diff --git a/skin.json b/skin.json index 7b66681..c692eb3 100644 --- a/skin.json +++ b/skin.json @@ -424,9 +424,6 @@ ], "scripts": [ "resources/skins.minerva.talk/init.js" - ], - "messages": [ - "minerva-talk-add-topic" ] }, "skins.minerva.toggling": { diff --git a/tests/browser/features/talk.feature b/tests/browser/features/talk.feature index 07f0315..cf95b31 100644 --- a/tests/browser/features/talk.feature +++ b/tests/browser/features/talk.feature @@ -4,40 +4,42 @@ Feature: Talk Background: Given I am using the mobile site - @smoke @integration @login - Scenario: Talk doesn't show on talk pages - Given the page "Talk:Selenium talk test" exists - And I am logged in as a user with a > 5 edit count - And I am on the "Talk:Selenium talk test" page - Then there should be no talk button - @login Scenario: Talk on a page that does exist Given the page "Talk:Selenium talk test" exists - And I am logged in as a user with a > 5 edit count + And I am logged into the mobile website And the page "Selenium talk test" exists When I click the talk button Then I should see the talk overlay @login Scenario: Talk on a page that doesn't exist (bug 64268) - Given I am logged in as a user with a > 5 edit count + Given I am logged into the mobile website And I am on a page that does not exist When I click the talk button Then I should see the talk overlay @smoke @integration @login - Scenario: Add discussion on talk page possible as logged in user + Scenario: Add discussion for talk page possible as logged in user Given the page "Talk:Selenium talk test" exists - And I am logged in as a user with a > 5 edit count + And I am logged into the mobile website And the page "Selenium talk test" exists When I click the talk button Then there should be an add discussion button + @smoke @integration @login + Scenario: Add topic button shows on talk pages for logged in users + Given the page "Talk:Selenium talk test" exists + And I am logged into the mobile website + And I am on the "Talk:Selenium UI test" page + And there should be an add discussion button + When I click the talk button + Then there should be an add discussion button + @integration Scenario: A newly created topic appears in the list of topics immediately Given the page "Talk:Selenium talk test" exists - And I am logged in as a user with a > 5 edit count + And I am logged into the mobile website And the page "Selenium talk test" exists When I click the talk button And no topic is present