Page MenuHomePhabricator

Use OOUI icons for FlaggedRevs
Closed, ResolvedPublic

Description

BeforeAfter

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 21 2017, 8:39 AM
BeforeAfter

Smth strange with padding, anyway.

Change 333329 had a related patch set uploaded (by Ladsgroup):
Use OOjs UI icons

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

@Iniquity: I fixed the padding now:

@Iniquity: I fixed the padding now:

Thanks! And what about the first picture?

It'll fix all of them. I was too lazy to take a new picture :D

It'll fix all of them. I was too lazy to take a new picture :D

Yep, sorry, my fault :)

@Ladsgroup Minor stylistic comments based on the provided screenshots:

  • While we're creating icons in true black (WMUI's Base0 color), we don't recommend using it as icon color. We rather try to embed it into its context, may it be the grey of the surrounding text or link color. Compare the OOjs UI ButtonWidgets with indicators and icons.
  • The arrow on the left should better be an indicator, like in the demos too. Seems unproportional/wrong size.

I forgot to mention, as there's currently no good way to use the fill property on SVGs within MediaWiki, I've started using WMUI Base rules on opacity to closely emulate the surrounding text colors and also add :hover and :active opacity modifiers to the icons.

The idea of using indicator was great. Now, it looks like this:


Regarding colors. We can go with "disabled" mode to make it grey but not sure about it. Here's how it looks:

Also I like more contrast (so I like black).

@Ladsgroup I like the indicator!
Keep in mind, that not everything needs to be full contrast in order to enable users (together with other visual cues) to focus on the most important interface elements in the viewport. I don't think disabled would be the way to go either, as it's a visual cue in another interface direction, but something like opacity: 0.87 to provide a tad lower, still sufficient, contrast would be useful IMO.

It'll be like this with opacity of 0.87:


Not very much different though.

Would you provide a screenshot in bigger context?

So this is meant to be an element that triggers a menu/dropdown? Similar to a dropdown handler?

Fito added a subscriber: Fito.Feb 2 2017, 4:00 AM

Even though I agree we should rewrite UI of Flagged revisions, I think this is outside scope of this task, this task about cleaning up icons, OOUIfying FlaggedRevs should be done in other patches/steps.

@Ladsgroup It's getting closer, but could be also confusing have a visually related UI pattern, that is looking and behaving different though. I agree, that it's prob outside of this task's scope though.

Change 333329 merged by jenkins-bot:
Use OOjs UI icons

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

What's the state of FlaggedRevs in production?
I see the extension in installed but i'm not sure whether it is enabled.

This change appears to load all of OOjs UI's styles (and scripts) on the beta cluster for all pages on article pages in mobile (T158329). I urge you to revert this if this is going to roll out to all wikis.

What's the state of FlaggedRevs in production?
I see the extension in installed but i'm not sure whether it is enabled.

Loaded on about 5% of them.

This change appears to load all of OOjs UI's styles (and scripts) on the beta cluster for all pages on article pages in mobile (T158329). I urge you to revert this if this is going to roll out to all wikis.

Yeah, that needs fixing; it should only load on desktop (as mobile doesn't use the imagery), and only on pages which are flagged (which I thought it was doing but it appears not).

Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptFeb 16 2017, 5:22 PM
Tgr added a subscriber: Tgr.Feb 16 2017, 8:32 PM

I don't think mobile exposes flagrev status at all. (It probably should but that's a separate issue.)

I don't think mobile exposes flagrev status at all. (It probably should but that's a separate issue.)

https://en.m.wikipedia.beta.wmflabs.org/wiki/PC1 shows "The latest accepted version was accepted on 3 February 2014. There is 1 pending revision awaiting review." for me when logged in, but nothing when IP; there's no control to review different versions or iconography, however.

Tgr added a comment.Feb 16 2017, 8:52 PM

Thanks. It indeed (partially) shows when logged in.

Change 338215 had a related patch set uploaded (by Ladsgroup):
Re-introduce "Use OOjs UI icons"

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

Change 338215 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/FlaggedRevs@master] Re-introduce "Use OOjs UI icons"

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

Ladsgroup updated the task description. (Show Details)May 17 2019, 6:09 PM
Ladsgroup updated the task description. (Show Details)May 17 2019, 10:06 PM

When will it be deployed? When ready, please move it to "Announce in next Tech News" on the User-notice board.

When will it be deployed? When ready, please move it to "Announce in next Tech News" on the User-notice board.

Once it's merged, I moved it there.

Change 338215 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Re-introduce "Use OOUI icons"

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

Ladsgroup updated the task description. (Show Details)Mon, Jun 3, 10:56 AM

Change 514200 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/FlaggedRevs@master] Update icons to OOUI's latest and also update license

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

Change 514200 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Update icons to OOUI's latest and also update license

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

Volker_E closed this task as Resolved.Wed, Jun 5, 11:48 PM
Volker_E removed a project: Patch-For-Review.
stjn added a subscriber: stjn.Thu, Jun 6, 7:21 PM

@Volker_E: two issues, not sure if I need to open new tasks.

a) ‘Eye’ icon is badly aligned when you have ‘Use detailed boxes to show review status of pages’ enabled in Preferences → Recent changes (I guess this is more an issue with OOUI).

Reproduce: https://ru.wikipedia.org/wiki/Русские_переводы_«Божественной_комедии»

b) The icons visibly change in size on first page load and have different size if JavaScript is disabled. Why does this happen?

Reproduce: https://ru.wikipedia.org/wiki/Кох,_Ганс

stjn awarded a token.Thu, Jun 6, 7:21 PM
Volker_E added a comment.EditedThu, Jun 6, 7:49 PM

@stjn First issue should be resolved with new version rolling out to all wikis beginning next Tuesday.
Also, second issue is going to disappear with caching going away. There was a last minute icon update as the original patch was so long in the making that it relied on icons before T177432. In non-JS you see the old one with 24x24px canvas, in the JS version the 20x20px. The eye is also featuring the old canvas and misalignment within.

Update
I got poked, that due to an offsite, there will be no deployment next week. So it will take 2 weeks before the latest patch take full effect.

stjn added a comment.Thu, Jun 6, 7:54 PM

Thank you for quick response

stjn added a comment.Fri, Jun 7, 12:24 PM

Big regression introduced with this change: the diff that shows to FlaggedRevs reviewers when there are any unreviewed changes can’t be shown anymore because diff toggle gets hidden upon loading. Some reviewers probably won’t be happy with this. I don’t know how we’re going to fix this if there won’t be any deployments for 2 weeks, though.

(Ideally, this old code shouldn’t use bootleg toggles and should use jquery.makeCollapsible, but alas.)

Reproduce (look for #mw-diff-toggle in browser console):
https://ru.wikipedia.org/wiki/Кох,_Ганс?action=edit

Change that caused this:
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/FlaggedRevs/+/338215/22/frontend/modules/ext.flaggedRevs.advanced.js

Iniquity reopened this task as Open.Sat, Jun 8, 11:46 AM
Iniquity added a project: Regression.
Ladsgroup closed this task as Resolved.
Ladsgroup removed a project: Regression.

I made T225351: Regression in diffing unreviewed changes in edit mode in FlaggedRevs for the regression. The point of the task is done (Renewing icons). It should be reopened if the regression causes it to be reverted which haven't happened yet (it's more likely that this regression gets fixed instead)

Gilles removed a subscriber: Gilles.Tue, Jun 11, 8:28 AM