Page MenuHomePhabricator

Page banner should not render on revisions
Closed, ResolvedPublic

Description

See https://en.wikivoyage.org/w/index.php?title=London&type=revision&diff=2843793&oldid=2843199
The banner appears above the diff.
Expected: it should not appear at all.

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task to Epics on the Wikidata-Page-Banner board.
Jdlrobson added a subscriber: Jdlrobson.
Restricted Application added a project: Wikidata. · View Herald TranscriptAug 26 2015, 7:42 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdlrobson moved this task from Epics to Backlog on the Wikidata-Page-Banner board.Aug 28 2015, 7:28 PM
Jdlrobson moved this task from Backlog to Epics on the Wikidata-Page-Banner board.Aug 30 2015, 5:03 PM
Sumit added a subscriber: Sumit.EditedSep 1 2015, 5:13 PM

Is there a way to detect that we are viewing a diff page, other than resorting to ArticleContentOnDiff hook.

You could check for the existence of the diff parameter that should be enough.

		$diff = $request->getVal( 'diff' );

Change 235492 had a related patch set uploaded (by Sumit):
WikidataPageBanner disable on diff pages

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

Tgr added a subscriber: Tgr.Sep 2 2015, 5:35 PM

Is that a good idea? Patrolling changes to the pagebanner parameters seems kind of annoying without seeing the effect. The banner should be on top of the article, not top of the page, but otherwise it seems useful to have it.

Change 235492 merged by jenkins-bot:
WikidataPageBanner disable on diff pages

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

@Nicolas_Raoul thoughts on @Tgr's comment [1]? Should banners be showing underneath the diff itself?
@Sumit any idea why they were showing up where they were?

[1] https://phabricator.wikimedia.org/T110384#1598347

From a user experience point of view, I agree with Tgr that the banner should appear between the diff (or source edit box) and the actual page content, so that the preview is as close to reality as possible.

Sumit added a comment.Sep 4 2015, 3:12 AM

Banner has been showing above diff because by nature it is meant to be the first element of page body. For this it uses OutputPage::prependHtml() on page body. In case of a diff, it does not distinguish it from other content.

Mmm... is there any way we can add it when encountered rather than use prependHtml?
I guess this will cause problems with multiple uses of the banner code or adding a banner at the bottom of the page causing it to render at the bottom?

How does the diff get added?
It's worth noticing the mobile diffs do not give a preview at all.

Using the mechanism in https://gerrit.wikimedia.org/r/#/c/238654/ we should be able to inject it under the revisions... in theory... not sure how problematic not having this is to the community in the meantime.

Jdlrobson closed this task as Resolved.Sep 23 2015, 6:08 PM
Jdlrobson claimed this task.

On reflection given that the page headings do not render in revisions and the banner is substitution for the heading I actually think this is the correct behaviour. For instance the heading "Download" does not show up in https://www.mediawiki.org/w/index.php?title=Download&type=revision&diff=1896349&oldid=1813316

If we do want to show the banner in the revision diff somewhere someone should raise a separate feature request for that and we should get some design input.