Page MenuHomePhabricator

Minerva: Add support for SkinAfterContent hook
Closed, ResolvedPublic

Description

The normal approach for extensions to add crap after the content in any given skin.

MF can probably just kill anything bad for mobile; the skin should support it regardless.

Event Timeline

Change 517959 had a related patch set uploaded (by Isarra; owner: Isarra):
[mediawiki/skins/MinervaNeue@master] Add DataAfterContent to footer (SkinAfterContent hook)

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

phuedx triaged this task as Medium priority.Jun 20 2019, 10:09 AM

This change is correct, but most probably we should fix how the FlaggedRevs looks like on Mobile first (it's the only extension on production that uses SkinAfterContent hook to inject something to the page).

That's how it looks when Minerva supports SkinAfterContent hook:

image.png (637×1 px, 36 KB)

I created T226492: FlaggedRevs form styles missing on Minerva skin to fix Flagged revs form styles.

Is minerva missing fieldset styles?

Okay, that was the wrong place for that comment, so let's follow up with another really wrong-placed comment - I should also probably be rechecking all the positioning for FR in the other skins if we're maybe going to move all that there too...

Is minerva missing fieldset styles?

Minerva doesn't load all desktop styles. The SkinMinerva::getDefaultModules just resets all $modules['styles']['core'] and loads only some mediawiki styles (like mediawiki.hlist, mediawiki.ui.icon, mediawiki.ui.button and wikimedia.ui)

Adding to Kanbanana board so Readers-Web-Team is aware of this change

Moving to block on others - once this patch rides on train we should verify that mobile footer on production looks good.

Change 517959 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Add DataAfterContent to footer (SkinAfterContent hook)

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

Jdlrobson subscribed.

What needs testing here? Do we now where this hook is run and what could go wrong?

@Ladsgroup heads up that FlaggedRevs will now show up in Minerva skin (see T226143#5281819)

@Ladsgroup heads up that FlaggedRevs will now show up in Minerva skin (see T226143#5281819)

My personal opinion: Burn flagged revs to the ground and rewrite it from scratch.
My more professional opinion: By doing a little bit of refactoring of FlaggableXML, you can probably stop using the SkinAfterContent and achieve your goal

By doing a little bit of refactoring of FlaggableXML, you can probably stop using the SkinAfterContent and achieve your goal

Why? What goal, exactly?

@Jdlrobson we need to verify that pages on production still looks good, I left this task on Kanbanana board to so we do not forget to QA it.
There is no easy way to test this because we do not have an environment that exactly matches production (beta cluster do not have FlaggedRevs extension).