Page MenuHomePhabricator

Deprecate Skin::subPageSubtitle as a public method
Closed, ResolvedPublic

Description

As we evolve the skin architecture, we are trying to reduce the amount of functions that skins can extend to reduce the possibility of divergence in HTML marks. Currently subtitles use inconsistent markup meaning extensions have to make use of skinStyles/Scripts to target them in different skins. See also T253813. We thus plan to remove this function.

  • In SkinTemplate Skin::subPageSubtitle is called inside SkinTemplate::prepareSubtitle and provided via the Template variable subtitle
  • In the new SkinMustache this is provided as the template variable html-subtitle

Core code provides subtitle inside a span element with class "subpages":
https://gerrit.wikimedia.org/g/mediawiki/core/+/27076b5e34b2b3302fbfb64da9dbeb3101a89b4a/includes/skins/SkinTemplate.php#194

Despite this Nostalgia uses the method directly

$subpages = $skin->subPageSubtitle();
494		$sub .= !empty( $subpages ) ? "</p><p class='subpages'>$subpages" : '';
495		$s = "<p class='subtitle'>{$sub}</p>\n";

CologneBlue also uses this directly:

<?php
352		if ( $this->getSkin()->subPageSubtitle() ) {
353			?>
354			<p class="subpages"><?php echo $this->getSkin()->subPageSubtitle() ?></p>
355		<?php
356		}

Acceptance criteria

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 30 2020, 10:51 PM
Jdlrobson triaged this task as Medium priority.Sep 4 2020, 9:28 PM

Change 625580 had a related patch set uploaded (by Jdlrobson; owner: Ammarpad):
[mediawiki/skins/CologneBlue@master] Use prepared and combined subpage/page subtitle

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

Ammarpad added a subscriber: Ammarpad.EditedSep 10 2020, 6:46 AM
  • Update VisualEditor to call prepareSubtitle

It's not possible to call this method from VisualEditor currently due to its restricted visibility

JTannerWMF added a subscriber: JTannerWMF.

@Jdlrobson when will you need this reviewed by the Editing team, also can you add some thought around the motivation for this change?

Jdlrobson updated the task description. (Show Details)Sep 10 2020, 4:28 PM
Jdlrobson added a comment.EditedSep 10 2020, 8:02 PM
  • Update VisualEditor to call prepareSubtitle

It's not possible to call this method from VisualEditor currently due to its restricted visibility

Let's make SkinTemplate::prepareSubtitle public then. I'd rather have a public SkinTemplate::prepareSubtitle then as it would also simplify the following:

$subpagestr = $this->getSkin()->subPageSubtitle( $tempOut );
				if ( $subpagestr !== '' ) {
					$subpagestr = '<span class="subpages">' . $subpagestr . '</span>';
				}
				$result['contentSub'] = $subpagestr . $this->getOutput()->getSubtitle();

to one line:

				$result['contentSub'] = $this->getSkin()->prepareSubtitle();

@Jdlrobson when will you need this reviewed by the Editing team, also can you add some thought around the motivation for this change?

I can take care of code review for this one @JTannerWMF if the editing team is happy with that.

Change 625580 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/skins/CologneBlue@master] Use prepared and combined subpage/page subtitle

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

Ammarpad updated the task description. (Show Details)Sep 12 2020, 7:18 AM

Change 625580 merged by jenkins-bot:
[mediawiki/skins/CologneBlue@master] Use prepared and combined subpage/page subtitle

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

Jdlrobson updated the task description. (Show Details)Sep 14 2020, 8:09 PM
JTannerWMF moved this task from To Triage to Triaged on the VisualEditor board.Sep 22 2020, 3:48 PM

Hey @Jdlrobson , if you can perform code review that would be awesome. Editing can't prioritize this task right now.

Change 629243 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Make SkinTemplate::prepareSubtitle() public

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

Change 629243 merged by jenkins-bot:
[mediawiki/core@master] Make SkinTemplate::prepareSubtitle() public

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

Jdlrobson updated the task description. (Show Details)Sep 23 2020, 4:53 AM

Change 629250 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/extensions/VisualEditor@master] Use Skin::prepareSubtitle()

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

Change 629254 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/skins/Nostalgia@master] Use Skin::prepareSubtitle()

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

Ammarpad added a comment.EditedSep 23 2020, 6:50 AM

Mark the Skin::subPageSubtitle method as deprecated and move its content to a private Skin method.

So think there's some complication here. What will happen to SkinTemplate::prepareSubtitle() then? If Skin::subPageSubtitle is turned to private, SkinTemplate method (the recommended alternative, cannot call it either). If it duplicates the logic, which is bad itself, it will render the private Skin method unused at all.

Probably SkinTemplate::prepareSubtitle() needs to be moved to Skin to move this forward.

Change 630885 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Move prepareSubtitle() method to Skin

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

Change 630885 merged by jenkins-bot:
[mediawiki/core@master] Move prepareSubtitle() method to Skin

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

Change 629254 merged by jenkins-bot:
[mediawiki/skins/Nostalgia@master] Use Skin::prepareSubtitle()

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

Change 631257 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Turn Skin::subPageSubtitle into a private method

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

Ammarpad updated the task description. (Show Details)Sep 30 2020, 7:09 PM

Change 637032 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] ApiParse: Add 'subtitle' option

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

Change 637033 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ApiVisualEditorEdit: Use action=parse 'subtitle' option

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

Change 637032 merged by jenkins-bot:
[mediawiki/core@master] ApiParse: Add 'subtitle' option

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

Change 637033 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ApiVisualEditorEdit: Use action=parse 'subtitle' option

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

Jdlrobson updated the task description. (Show Details)Fri, Oct 30, 5:26 PM

Change 629250 abandoned by Bartosz Dziewoński:
[mediawiki/extensions/VisualEditor@master] Use Skin::prepareSubtitle()

Reason:
Replaced by https://gerrit.wikimedia.org/r/637033

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

Change 631257 merged by jenkins-bot:
[mediawiki/core@master] Turn Skin::subPageSubtitle into a private method

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

Jdlrobson closed this task as Resolved.Wed, Nov 18, 5:16 PM
Jdlrobson claimed this task.
Jdlrobson awarded a token.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptWed, Nov 18, 5:16 PM