Page MenuHomePhabricator

Convert FlaggedRevisions to Codex
Closed, ResolvedPublic

Assigned To
Authored By
Volker_E
Mar 31 2018, 10:35 PM
Referenced Files
F57282452: image.png
Aug 21 2024, 12:07 PM
File Not Attached
F57283910: image.png
Aug 21 2024, 11:33 AM
F57283816: grafik.png
Aug 21 2024, 9:50 AM
F57268961: Screenshot 2024-08-12 at 11.12.31 AM.png
Aug 12 2024, 9:16 AM
Restricted File
Aug 11 2024, 4:36 PM
Restricted File
Aug 11 2024, 4:36 PM
F57267738: Screenshot from 2024-08-11 18-21-10.png
Aug 11 2024, 4:22 PM
F57267588: image.png
Aug 11 2024, 2:39 PM
Tokens
"Heartbreak" token, awarded by Pppery."Yellow Medal" token, awarded by Aklapper.

Description

Due to the fact that FlaggedRevisions does not use OOUI Codex the experience on mobile + Minerva is bad. We should port it to OOUI Codex to fix that.

Convert FlaggedRevision interface elements to OOUI Codex.
Example:

T191156 FlaggedRevs _before.png (180×2 px, 22 KB)

Details

Other Assignee
Ladsgroup
SubjectRepoBranchLines +/-
mediawiki/extensions/FlaggedRevsmaster+619 -409
mediawiki/extensions/FlaggedRevsmaster+13 -13
mediawiki/extensions/FlaggedRevsmaster+25 -27
mediawiki/extensions/FlaggedRevsmaster+681 -170
mediawiki/extensions/FlaggedRevsmaster+8 -5
mediawiki/extensions/FlaggedRevsmaster+1 -1
mediawiki/extensions/FlaggedRevsmaster+11 -17
mediawiki/extensions/FlaggedRevsmaster+2 -2
mediawiki/extensions/FlaggedRevsmaster+28 -72
mediawiki/extensions/FlaggedRevsmaster+3 -3
mediawiki/extensions/FlaggedRevsmaster+10 -6
mediawiki/extensions/FlaggedRevswmf/1.43.0-wmf.15+24 -34
mediawiki/extensions/FlaggedRevsmaster+24 -34
mediawiki/extensions/FlaggedRevswmf/1.43.0-wmf.15+2 -1
mediawiki/extensions/FlaggedRevsmaster+2 -1
mediawiki/extensions/FlaggedRevsmaster+48 -35
mediawiki/extensions/FlaggedRevsmaster+1 -10
mediawiki/extensions/FlaggedRevsmaster+14 -7
mediawiki/extensions/FlaggedRevsmaster+55 -16
mediawiki/extensions/FlaggedRevsmaster+154 -45
mediawiki/extensions/FlaggedRevsmaster+57 -23
mediawiki/extensions/FlaggedRevsmaster+414 -39
mediawiki/extensions/FlaggedRevsmaster+48 -75
mediawiki/extensions/FlaggedRevsmaster+99 -190
mediawiki/extensions/FlaggedRevsmaster+271 -248
mediawiki/extensions/FlaggedRevsmaster+3 -2
mediawiki/extensions/FlaggedRevsmaster+217 -133
Show related patches Customize query in gerrit

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Misuse of inline message style leads to another huge bug: entire ‘There are changes since the page was last reviewed’ message gets bolded, including the diff:

image.png (801×1 px, 108 KB)

I reported this already in T371681. The fix should go live this week.

In ‘detailed’ mode (see Preferences § Recent changes), there is a FlaggedRevs icon in code sometimes that isn’t showing up at all:

image.png (635×1 px, 103 KB)

It needs to either be removed, moved to be Codex message icon or shown again.

Change #1059456 had a related patch set uploaded (by Abaris; author: Abaris):

[mediawiki/extensions/FlaggedRevs@master] Refactor and clean up FlaggedRevs UI elements and CSS selectors

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

BeforeAfter

The changed popup looks a bit too large for me. The last sentence could be omitted IMO, it doesn't provide any relevant information.

By the way, the fact that in the new "design" of the "Pending Changes" table the column headings "size" and "since" have a text size of 1rem, and "page" and "watching" - 100% = 1em - is this done on purpose? Some Johannes89 will say "I like the updated design", but it's not pretty, right?

Please describe what it "breaks", in words.

Screenshot from 2024-08-11 18-21-10.png (684×1 px, 219 KB)

Edit: Ah, this seems to only affect Vector-2022, sorry.

Hello @Iniquity, thanks for your feedback. There is an upcoming patch that includes a new indicator view. Please see: T191156#10055466

Another problem with the changes: https://ru.m.wikipedia.org/wiki/Playboi_Carti no longer includes any indication that there are unreviewed changes, https://ru.wikipedia.org/wiki/Playboi_Carti at the same time does. This was previously not the case and is a Regression.

UPD: it is only like that for logged in users. See the screenshot from another user:

{F57267752}

Additionally, for the logged out users, the message they get lacks any classes that would allow to customise how it looks. That should also be fixed. Currently interface administrators have no way to fix the design locally on the mobile version. Previously it was possible.

Please consider making separate Phab tickets for each of these action items such as adding HTML classes to X, or logged in users not being able to see the review chip on mobile, so that they are not forgotten and so things are more organized.

As a dev, I find it much easier when 1 ticket = 1 patch, rather than having to tease the tickets out of a big thread like this one. And if things are easier for devs they are more likely to get done :)

I get the point, but I think that the responsibility of creating the tasks for something that was not broken before should lie with the person who broke it. I reported around 10 issues here already, creating a task for every single one of them is extremely tedious.

It's not much more complex than commenting on a task. There are reasons why there is often more than one single catch-all task for each software project. :P

Another problem with the changes: https://ru.m.wikipedia.org/wiki/Playboi_Carti no longer includes any indication that there are unreviewed changes, https://ru.wikipedia.org/wiki/Playboi_Carti at the same time does. This was previously not the case and is a Regression.

UPD: it is only like that for logged in users. See the screenshot from another user:

{F57267752}

Hello @stjn, It's visible in the current version; you might consider displaying it again.

Screenshot 2024-08-12 at 11.12.31 AM.png (690×2 px, 201 KB)
#mw-fr-revisiontag {
    display: none;
}

I get the point, but I think that the responsibility of creating the tasks for something that was not broken before should lie with the person who broke it. I reported around 10 issues here already, creating a task for every single one of them is extremely tedious.

You can use the format

[] ...short explanation of the issue...

to add issues to a checkbox list in the description of this task, with a link to the relevant comment. That's not much more effort than just commenting and makes it easier to keep track of what has been reported and what has been solved.

Hello @stjn, It's visible in the current version; you might consider displaying it again.

Thanks, my bad. Previously the CSS rule didn’t apply. Fixed it for the current version.

I would note though that this was previously much more prominent for mobile version. We in Russian Wikipedia intend to fix this by switching to ‘detailed’ mode since some changes to ‘compact’ mode made it much worse: https://ru.wikipedia.org/wiki/Википедия:Форум/Общий#Исправление_стандартного_отображения_информации_о_патрулировании

It's not much more complex than commenting on a task. There are reasons why there is often more than one single catch-all task for each software project. :P

It is.

Change #1059456 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Refactor and clean up FlaggedRevs UI elements and CSS selectors

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

Tech news time again:

The indicators at the top of articles shown by flagged revs (also known as pending changes) have changed to be more standardized.

https://doc.wikimedia.org/codex/latest/components/demos/table.html#heading-content

Do not rely on iconography alone to represent categorical information in headings

The use of the eye icon as a column header seems to violate this.

I don't think we do that? The new design as you can see in https://en.wikipedia.beta.wmflabs.org/wiki/Mavetuna1 shows text next to the icon:

grafik.png (161×495 px, 9 KB)

There might be some option that hides the text and if that's the case, we need to completely remove that option and makes unconditionally show the text all the time.

I'm talking about Special:PendingChanges.

Tech news time again:

The indicators at the top of articles shown by flagged revs (also known as pending changes) have changed to be more standardized.

  • Bug: If I hover over the indicator, the popup appears, but if I move the cursor away, it doesn’t disappear, unless I move it over the popup. Why does it use hover at all? I don’t think anything else on Vector 2022 uses hover – even the More tab, which worked on hover in Vector 2010, is now click-only. Not working on hover would naturally fix this bug, and at the same time it would remove the annoying popup being opened accidentally (since it’s now more than five times as huge as the previous one, it’s much more annoying).
  • Design: The (non-dismissable) notice on mobile is pretty big, both compared to desktop and compared to the previous version. And it even appears on the main page if that happens to be in mainspace.
  • Accessibility: It doesn’t work without JavaScript. This is no step back (neither did the previous version), but it isn’t a step forward either; it still means that Grade C browsers may not get content presented in a readable manner.
  • Accessibility: It cannot be reached using the keyboard. In fact, the indicator isn’t even marked up to be an interactive element. No focusable-by-default and interactive-by-default element (<button>, <a> etc.), no tabindex attribute, no role attribute. (Like the previous point, this is no step back but no step forward either.)

Using a <detail> with a <summary class="cdx-button cdx-button--weight-quiet"> could probably solve the first, third and fourth points. Take a look at the Repos column on Patch Demo for how much can be achieved with just a <detail> and some CSS.

For the second point, the perfect solution would be T75299: Indicators (protected icon, featured icon) are not shown in Minerva, but I hope it can be tweaked even without that.

Having two Cancel buttons doesn’t seem all that good either:

image.png (290×590 px, 23 KB)

That part doesn’t annoy me much, but I wouldn’t mind removing the Cancel button either. Although F57282452 shows that there’s another button for reviewers, probably keeping that and removing Cancel wouldn’t be too unbalanced either.

We could simply not show it if the user doesn't have review right. That would also make this much smaller in most use cases.

For Tech News, is the proposed draft still suitable as an announcement, or does it need any more details? (Thanks for the draft!).
Here's a tweaked draft (with speculated 2nd sentence! Please approve/tweak as needed):

Editors at wikis that use flagged revisions (also known as pending changes) may notice that the indicators at the top of articles have changed to be more standardized. There are some more expected improvements to these interface components, coming soon. [1]

Also, is there anything good we can link to, or just this task? Thanks.

There are some more expected improvements to these interface components, coming soon

I think this is the last part, no more codexification left to do.

Also, is there anything good we can link to, or just this task? Thanks.

This ticket is good.

Would be good to have some attention on T372694: Switch ruwiki to use FlaggedRevs detailed interface mode and the issues I’ve raised in my comment there:

@Dogu can you check whether all users get Codex message styles in this mode?

https://en.wikibooks.org/wiki/Indonesian/Grammar/Adverbs (a wiki using this mode) doesn’t seem to look so for me logged out or logged in. The message lacks Codex styling.
https://fi.wikipedia.org/wiki/Kaukopuhelu?safemode=1 has the same issue.
On https://fi.wikipedia.org/wiki/Venäjän_vaakuna?safemode=1 the Codex styles load after the page gets loaded.

Change #1065147 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/FlaggedRevs@master] Hide Cancel button in the popup when the user doesn't have review right

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

Change #1065147 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Hide Cancel button in the popup when the user doesn't have review right

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

I'm closing this task because the latest updates have been deployed. If any issues arise in the future, a new task can be filed.

You’ve changed all the classes again without notifying anyone this would happen. I am opposed to it in principle but it is pretty annoying that this is not properly communicated, especially when it comes to CSS classes and IDs that have been used for 10 years or more.

https://www.mediawiki.org/wiki/Stable_interface_policy/Frontend#What_is_not_stable?

HTML markup and CSS cannot be considered stable (unless explicitly stated). All frontend code (including wiki-hosted code, extensions and skins) relying on HTML structure does so at its own risk

I am aware of that policy. But that policy doesn’t substitute for being a good sport to each other. Generally, when established classes are changed or are expected to change, people inform each other of that because that’s a good thing to do, not because of some policy. Documenting your changes for others makes it easier to make updates like https://ru.wikipedia.org/?diff=139868918 without requiring local volunteers to waste their time looking for new classes.

@stjn, I understand your point about documentation. If you're willing to handle the documentation for these changes, feel free to do so. I’ve been contributing my time and efforts voluntarily as well, and any help with documenting would be appreciated.