Page MenuHomePhabricator

Add styling for review notice from FlaggedRevs in MobileFrontend
Closed, ResolvedPublic

Description

Would like to report a small problem from not having Mobile.css render-blocking: we in Russian Wikipedia style default review notice from FlaggedRevs in some way because it is unstyled by default and quite big, so all headers of articles with unreviewed edits get a visible jump from this.

Reproduce: https://ru.m.wikipedia.org/wiki/Зыкин,_Владимир_Александрович

Maybe MobileFrontend could include this code somewhere (or FlaggedRevs could) so that we don’t have to?

#mw-fr-reviewnotice {
    color: #72777d;
    font-size: 0.8em;
    font-style: italic;
}

You can come up with something different, of course (I don’t like it myself, italic is a bit unnecessary here).

FlaggedRevs is being used in 45 Wikimedia projects and number of unreviewed pages is quite high in most of them, so this must be quite a serious visual problem (since unreviewed notice for editors is not that big of a deal to show in such way as default state does).

Details

Related Gerrit Patches:
mediawiki/extensions/FlaggedRevs : masterFix loading of default styles in Minerva
mediawiki/extensions/FlaggedRevs : masterAdd styling to review notices in Minerva

Event Timeline

stjn created this task.Jun 22 2018, 12:02 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 22 2018, 12:02 AM
stjn updated the task description. (Show Details)Jun 22 2018, 12:06 AM
Jdlrobson added a subscriber: Jdlrobson.

FlaggedRevs would be the best place. It could use skinStyles to register this rule for Minerva. Happy to help review!

Jdlrobson added a subscriber: alexhollender.

Any thoughts on how this should look @alexhollender ?

Jdlrobson moved this task from Needs triage to Triaged on the Mobile board.Jun 27 2018, 12:11 AM
Vvjjkkii renamed this task from Add styling for review notice from FlaggedRevs in MobileFrontend to ohaaaaaaaa.Jul 1 2018, 1:02 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from ohaaaaaaaa to Add styling for review notice from FlaggedRevs in MobileFrontend.Jul 2 2018, 11:04 AM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.

Change 445636 had a related patch set uploaded (by Saint Johann; owner: Saint Johann):
[mediawiki/extensions/FlaggedRevs@master] Add styling to review notices in Minerva

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

Change 445636 merged by Jdlrobson:
[mediawiki/extensions/FlaggedRevs@master] Add styling to review notices in Minerva

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

stjn added a comment.Jul 30 2018, 2:44 PM

@Jdlrobson: seems like I should’ve asked you for a screenshot when you tested this. It’s been finally deployed and it looks like this in the wild:
https://ru.m.wikipedia.org/wiki/Счисление_координат

The difference is in that I kind of thought that I was adding styles just for Minerva, not for all skins and Minerva. It’s not necessarily really bad (maybe default centre alignment looks a bit wrong), but as you are more proficient in this stuff: are you sure adding all styles from FlaggedRevs by default is not detrimental to performance or anything like that (so, should we fix this or not before resolving this task)?

The difference is in that I kind of thought that I was adding styles just for Minerva, not for all skins and Minerva.

I'm not sure I follow...
According to https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/FlaggedRevs/+/445636/7/FlaggedRevs.php,unified the styles you added will only apply to Minerva where ext.flaggedRevs.basic is loaded (since shipped via skinStyles)

The change in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/FlaggedRevs/+/445636/7/frontend/FlaggablePageView.php,unified removes the nomobile class so reveals it on mobile.

Can you take a screenshot of how you were expecting it should look?
Do we need to revert this patch in the meantime? If we do that today, it will be out this week.

stjn added a comment.Jul 30 2018, 2:56 PM

I don’t see a need in revert, since it doesn’t look critically bad.

What I was envisioning was something like this (i. e., I mistakenly thought that only Minerva styling would apply with that file, sorry):

What we got is something like this (only font-size rule was implemented because of my misunderstanding, common styles that are present on desktop started to load on mobile as well, which I didn’t take into account):

Oh I see.
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/FlaggedRevs/+/445636/7/FlaggedRevs.php,unified

For that you'll want to change this:

position'		=> 'top',
styles'        => [ 'ext.flaggedRevs.basic.css' ],
'skinStyles'    => [

to this:

position'		=> 'top',
'skinStyles'    => [
		'default' => [ 'ext.flaggedRevs.basic.css' ],
		'minerva' => [ 'ext.flaggedRevs.basic.minerva.css' ],
	],

Minerva will then replace the 'default' styles.
'default' styles will apply to all other skins.

Change 449888 had a related patch set uploaded (by Saint Johann; owner: Saint Johann):
[mediawiki/extensions/FlaggedRevs@master] Fix loading of default styles in Minerva

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

stjn added a comment.Aug 1 2018, 9:56 PM

Minerva will then replace the 'default' styles.
'default' styles will apply to all other skins.

Done, thanks so much for helping me with this. After merging the patch above this task can be marked as resolved.

Will take a look today !

With your most recent change I see:



Look good?

Change 449888 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Fix loading of default styles in Minerva

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

stjn added a comment.Aug 2 2018, 2:23 PM

Yes, thanks.

Jdlrobson closed this task as Resolved.Aug 2 2018, 3:55 PM
Jdlrobson claimed this task.