Maniphest T197915

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

Assigned To

Authored By

	stjn
	Jun 22 2018, 12:02 AM

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

	Subject	Repo	Branch	Lines +/-
	Fix loading of default styles in Minerva	mediawiki/extensions/FlaggedRevs	master	+1 -1
	Add styling to review notices in Minerva	mediawiki/extensions/FlaggedRevs	master	+23 -1

Customize query in gerrit

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 added a project: Web-Team-Backlog.Jun 22 2018, 12:03 AM

Jack_who_built_the_house subscribed.Jun 22 2018, 12:04 AM

stjn updated the task description. (Show Details)Jun 22 2018, 12:06 AM

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

Any thoughts on how this should look @alexhollender ?

Iniquity subscribed.Jun 22 2018, 12:13 AM

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 added projects: CheckUser, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), Tamil-Sites, Gamepress, Hashtags, Jade, KartoEditor, Language-2018-Apr-June, New-Editor-Experiences, Mail, TCB-Team (now WMDE-TechWish).

• 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 removed projects: TCB-Team (now WMDE-TechWish), Mail, New-Editor-Experiences, Language-2018-Apr-June, KartoEditor, Jade, Hashtags, Gamepress, Tamil-Sites, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), CheckUser.

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

gerritbot added a project: Patch-For-Review.Jul 13 2018, 4:03 PM

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

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-07-24 (1.32.0-wmf.14)).Jul 23 2018, 2:00 AM

@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.

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

In T197915#4461854, @Jdlrobson wrote:

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:

Screen Shot 2018-08-02 at 2.50.40 PM.png (315×672 px, 39 KB)

Screen Shot 2018-08-02 at 2.50.56 PM.png (400×1 px, 107 KB)

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

ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-08-07 (1.32.0-wmf.16)); removed MW-1.32-notes (WMF-deploy-2018-07-24 (1.32.0-wmf.14)).Aug 2 2018, 7:00 AM

Yes, thanks.

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

Jdlrobson claimed this task.

MBinder_WMF moved this task from Design to Readers-Web-Kanbanana-Board-2018-19-Q1 on the Web-Team-Backlog board.Aug 23 2018, 12:00 AM

MBinder_WMF edited projects, added Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q1); removed Web-Team-Backlog (Design).

Jdlrobson moved this task from To Do to Ready for Signoff on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q1) board.Aug 29 2018, 5:24 PM

	F24436392: Screen Shot 2018-08-02 at 2.50.56 PM.png
	Aug 2 2018, 6:51 AM

	F24436391: Screen Shot 2018-08-02 at 2.50.40 PM.png
	Aug 2 2018, 6:51 AM

	F24333078: image.png
	Jul 30 2018, 2:56 PM

	F24333072: image.png
	Jul 30 2018, 2:56 PM

Add styling for review notice from FlaggedRevs in MobileFrontendClosed, ResolvedPublicActions

Description

Details

Event Timeline

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