Page MenuHomePhabricator

Main Page on a wiki doesn't have a Discussion page / Talk page on Mobile View
Closed, ResolvedPublic

Description

While reading this task: T174995 and it's discussion beneath, I realized something also in mobile view. The main page doesn't have a discussion button or link to talk page in mobile view but other page have. See below;

So this only happens for a wiki's main page? Is this intentional?

NOTE: This has been tested cross wiki (Meta-Wiki, MediaWiki, etc), same behavior.

Developer notes

Looking through the code history I think this is an oversight.
It was added in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/213263/1/includes/skins/SkinMinerva.php to disable watch link on main page and then swept into the generic logic in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/301104/1/includes/skins/SkinMinerva.php

The problem is "page actions" covers all the links at the top of the page and the button
isAllowedPageAction should be extended to allow "talk" buttons on the main page, but isAllowedPageAction needs to still return false on main page for edit, watch.

Feel free to have a go - make sure to add new tests and not to break the existing ones!

Testing instructions

Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page while logged in
Scroll to the bottom of the page
Ensure that there is a "Discussion" button

Details

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : masterAdd "Discussion" button to Main page on Mobile View

Event Timeline

D3r1ck01 created this task.Oct 6 2018, 8:16 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 6 2018, 8:16 PM
D3r1ck01 renamed this task from Main Page doesn't have a Discussion page / Talk page on Mobile View to Main Page on a wiki doesn't have a Discussion page / Talk page on Mobile View.Oct 6 2018, 8:16 PM
D3r1ck01 updated the task description. (Show Details)

This appears to be intentional:
https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/includes/skins/SkinMinerva.php#L231

You'll need to do some digging (back in the MobileFrontend repo to work out the motivation here) but I suspect it was to avoid spam on the main talk page. Regardless, the code should document the reasoning or the line should be removed.

In case this is intentional @Jdlrobson, I think the reasoning is valid, the code should be documented to avoid future confusions. So could I submit a patch for this with the reason you mentioned?

Looking through the code history I think this is an oversight.
It was added in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/213263/1/includes/skins/SkinMinerva.php to disable watch link on main page and then swept into the generic logic in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/301104/1/includes/skins/SkinMinerva.php

The problem is "page actions" covers all the links at the top of the page and the button
isAllowedPageAction should be extended to allow "talk" buttons on the main page, but isAllowedPageAction needs to still return false on main page for edit, watch.

Feel free to have a go - make sure to add new tests and not to break the existing ones!

Jdlrobson updated the task description. (Show Details)Oct 8 2018, 6:25 PM
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
ovasileva triaged this task as Medium priority.Oct 16 2018, 9:15 PM

Change 468836 had a related patch set uploaded (by D3r1ck01; owner: Alangi Derick):
[mediawiki/skins/MinervaNeue@master] Add "Discussion" button to Main page on Mobile View

https://gerrit.wikimedia.org/r/468836

D3r1ck01 claimed this task.Oct 21 2018, 3:15 PM
D3r1ck01 moved this task from Backlog to Doing [WIP] on the User-D3r1ck01 board.

@Jdlrobson, a patch is up for review :)

@D3r1ck01 patch looks good but I'm seeing an alignment issue

relates to

.page-Main_Page #page-secondary-actions .language-selector {
    margin-top: 1em;
}

so would be good to understand why that rule exists and if it's needed
(at minimum we'll want to apply the same for talk)

Hmm..... Didn't spot this. Please Jon, don't merge the patch. Let me include this UI fix too at once. Then the cleanup you suggested too before you do final review and possibly merge. But I'm glad the patch is in the right direction. Thanks :)

Thank YOU 🙏

@Jdlrobson, so that rule exist and is highly used :) So I've just applied same style that goes to .language-selector to the .talk which is a class for talk / discussion button. New CSS now look like;

.page-Main_Page #page-secondary-actions .talk .language-selector {
    margin-top: 1em;
}

I made sure .talk comes before .language-selector as it appears on the interface, but reversing their positions doesn't change anything on the UI, I just want code and UI to be in sync.

Also, I've reworked the logic a little bit and dried it up like you suggested but the logic gate is a little different now ;). You can review the PS on Gerrit :)

Change 468836 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Add "Discussion" button to Main page on Mobile View

https://gerrit.wikimedia.org/r/468836

@Jdlrobson, when is the right time to test on beta cluster? When it's finally deployed if I'm not making a mistake right? If so, does this need to go in SWAT?

No need for SWAT. This will ride the train. Your change is now live on https://en.m.wikipedia.beta.wmflabs.org/ !

@Jdlrobson, thanks the clarification about SWAT. Also, I've just checked https://en.m.wikipedia.beta.wmflabs.org/ and don't seem to find the "Discussion" button, do you see it on your end?

@Jdlrobson, thanks the clarification about SWAT. Also, I've just checked https://en.m.wikipedia.beta.wmflabs.org/ and don't seem to find the "Discussion" button, do you see it on your end?

Sorry about this! I was not logged in :), just logged in now and I see it. This is another call for concern, should the discussion button show only for logged in users? Usually, annon users should have access to this functionality right?

T54165: Links to talk pages in mobile view for all anonymous users

@Jdlrobson two clarifications:

alexhollender removed Ryasmeen as the assignee of this task.Oct 25 2018, 1:12 AM
alexhollender added a subscriber: Ryasmeen.

@Jdlrobson two clarifications:

Yup.

  • since we moved the Needs Design Review column, shall I move them to Needs Code Review after I'm done with them?

Yes

ABorbaWMF added a subscriber: ABorbaWMF.

This one is not working for me on https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page on the device simulators. I also tried on a couple of real devices and it is not working there either. It does work on regular desktop browsers.

@ABorbaWMF you need to be logged in to see the button. See T206406#4691583

Jdlrobson updated the task description. (Show Details)Nov 7 2018, 1:22 AM
phuedx removed Ryasmeen as the assignee of this task.Nov 8 2018, 6:05 PM
ovasileva closed this task as Resolved.Nov 8 2018, 9:14 PM
ovasileva claimed this task.
ovasileva added a subscriber: ovasileva.

looks good to me! resolving.

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptNov 8 2018, 9:14 PM