Page MenuHomePhabricator

Clean up the technical debt and inconsistencies of notification icon (Echo badge) positioning in different skins
Closed, ResolvedPublic

Description

Motivation

Visual: notification icons are the odd one in the header with most skins. Bit misaligned or over-sized, different focus outline (and slightly transparent).
Technical: changing alignment (margin, padding, font-size) of menu items in the header is very likely to change positioning of the notification icons in significantly different ways than menu items. The icons should behave consistently with menu items.

Requirements
  • Notification button sizes are proportional to the header and menu items (Modern, MonoBook). This means the skins should be responsible for rendering the icons Descoped. Monobook and Modern is in maintainance mode, see T141944
  • Button sizes are responsive to font-size changes (all skins).
  • Buttons' focus outline can be restored to browser default, consistent with any other link's behavior (all skins).
  • The button should work on desktop Minerva https://en.wikipedia.org?useskin=minerva
  • ~~The button should work on mobile Vector https://en.m.wikipedia.org?useskin=vector~~ descoped as this is a separate issue - no targets - handled by T127268
Related

This work started in the Vector skin: T256893: Move the PersonalMenu to the header
Modern skin: T246931: Modern echo icons too high up

Details

SubjectRepoBranchLines /-
mediawiki/extensions/Echomaster 84 -9
mediawiki/skins/MonoBookmaster 4 -0
mediawiki/extensions/Echomaster 0 -46
mediawiki/skins/MinervaNeuemaster 28 -0
mediawiki/skins/MonoBookmaster 13 -0
mediawiki/skins/Vectormaster 65 -15
mediawiki/skins/Vectormaster 98 -0
mediawiki/skins/Vectormaster 43 -6
mediawiki/extensions/Echomaster 0 -54
mediawiki/skins/Vectormaster 47 -0
mediawiki/extensions/Echomaster 14 -7
mediawiki/extensions/Echomaster 4 -0
mediawiki/extensions/Echomaster 2 -0
mediawiki/skins/MinervaNeuemaster 1 -146
mediawiki/skins/MinervaNeuemaster 118 -4
mediawiki/skins/Vectormaster 12 -0
mediawiki/skins/CologneBluemaster 0 -7
mediawiki/extensions/Echomaster 55 -37
mediawiki/skins/Vectormaster 11 -2
mediawiki/skins/MonoBookmaster 47 -4
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

Change 716609 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] WIP: Echo icons should be consistent with other Vector menu icons.

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

Change 716609 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] Echo icons should be consistent with other Vector menu icons.

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

Change 768168 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Add support for icons not prefixed with `wikimedia-`.

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

Change 768168 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Add support for icons not prefixed with `wikimedia-`.

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

kostajh subscribed.

@Jdlrobson when you'd like our review again, please move it back to the code review column on our current sprint board and/or leave a comment here. thanks!

Change 803369 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] WIP: Make Echo icons consistent with rest of Vector

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

Change 716609 abandoned by Jdlrobson:

[mediawiki/extensions/Echo@master] WIP: Echo icons should be consistent with other Vector/Minerva menu icons.

Reason:

Continuing in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/ /803369

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

Change 803998 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/CologneBlue@master] Drop Echo styles from CologneBlue

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

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/77fe56a465/w/

@alexhollender_WMF this is not a high priority right now, but I was hoping if we get time to fix this before shipping Vector 2022 further. I've started looking into this and was curious if you see the patchdemo has fixing the visual problems you have with the existing Echo icons. Note the larger touch areas and borders.

Test wiki created on Patch demo by TheresNoTime using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/dc7a0ec483/w/

@alexhollender_WMF this is not a high priority right now, but I was hoping if we get time to fix this before shipping Vector 2022 further. I've started looking into this and was curious if you see the patchdemo has fixing the visual problems you have with the existing Echo icons. Note the larger touch areas and borders.

Looking at Vector 2022 I don't understand what the visual issue is with these icon-buttons. Can somebody clarify? Or perhaps this is just a code issue?

Test wiki on Patch demo by TheresNoTime using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/dc7a0ec483/w/

@Jdlrobson it looks pretty good. compared with production the notices icon (inbox) is 1px off, and the alerts icon (bell) is 3px off:

image.png (620×1 px, 58 KB)

also, i'm not sure if this is a concern or not, but both icons are showing badges with a 0, and when I click on the icons the 0 shifts down which is incorrect:

Screen Shot 2022-06-14 at 11.07.06 AM.png (76×501 px, 4 KB)
Screen Shot 2022-06-14 at 11.07.18 AM.png (75×499 px, 4 KB)

Change 805488 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] Prepare for moving Echo code out of Minerva

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

Change 805489 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] Drops Minerva Echo code

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

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/9225eb4c81/w/

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/9225eb4c81/w/

Change 803998 abandoned by Jdlrobson:

[mediawiki/skins/CologneBlue@master] Drop Echo styles from CologneBlue

Reason:

A later discussion.. not a priority right now.

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

I confirm that the Growth team doesn't currently have the capacity to help with code reviews or any other required assistance here. The Growth team is comfortable with the Web team taking on this work.

Change 832370 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] Add icon definitions to Echo badges

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

Change 832372 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Limit upgrading of Echo icons to Visual enhancements feature flag

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

Change 832372 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Limit upgrading of Echo icons to Visual enhancements feature flag

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

Change 832378 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] [Visual enhancements next] Restores the badge styling to Echo

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

Change 832380 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] Echo should fire onInitialize event after rendering of badge

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

Change 832381 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] [Visual enhancements next] Make the Echo overlay open again

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

Change 803369 abandoned by Jdlrobson:

[mediawiki/extensions/Echo@master] Stop rendering the Echo icon - defer to skins

Reason:

Trying a new approach limited to Vector 2022 with minimal Echo changes (https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /832378)

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

Change 832382 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] Update counter-num and counter-text attributes if present on change

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

Change 805488 abandoned by Jdlrobson:

[mediawiki/skins/MinervaNeue@master] Prepare for moving Echo code out of Minerva

Reason:

Focusing on Vector 2022 for now.

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

Change 805489 abandoned by Jdlrobson:

[mediawiki/skins/MinervaNeue@master] Drops Minerva Echo code

Reason:

Focusing on Vector 2022 for now.

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

Change 832382 abandoned by Jdlrobson:

[mediawiki/extensions/Echo@master] Update counter-num and counter-text attributes if present on change

Reason:

Not needed. Moved to https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /832381 [Visual enhancements next] (using ext.echo.badge.countChange hook which is less confusing.)

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

Change 832380 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Echo should fire onInitialize event after rendering of badge

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

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/df7bc7be01/w

Change 834601 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] Drop badge styles for Vector

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

Change 834603 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Echo: Move skinStyles from Echo extension to Vector

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

Change 832370 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Add icon definitions to Echo badges

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

Change 834603 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Echo: Move skinStyles from Echo extension to Vector

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

Change 834601 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Drop badge styles for Vector

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

Change 834396 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] [[2]Visual enhancements next] Make the Echo buttons functional

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

Change 832378 merged by jenkins-bot:

[mediawiki/skins/Vector@master] [Visual enhancements next] Restores the badge styling to Echo

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

Change 832381 abandoned by Jdlrobson:

[mediawiki/skins/Vector@master] [[1]Visual enhancements next] Make the Echo buttons functional

Reason:

Asked Jan to weigh in here and we have decided to go with https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /834396 as the setTimeout logic is a little overly complicated.

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

Change 834396 merged by jenkins-bot:

[mediawiki/skins/Vector@master] [Visual enhancements next] Make the Echo buttons functional

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

Change 858666 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] Move skinStyle from Echo to Minerva

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

Change 858667 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] Move skinStyle rules to skin

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

Change 858668 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MonoBook@master] Move skinStyles for Echo to Monobook

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

Change 858669 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MonoBook@master] Fix alignment when Echo is clicked

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

Change 858668 merged by jenkins-bot:

[mediawiki/skins/MonoBook@master] Move skinStyles for Echo to Monobook

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

Jdlrobson updated the task description. (Show Details)

Calling this done with the above changes. Skins now own the rendering of Echo icons.

Echo will own underlying functionality e.g. anything inside overlays. 

For more specifics about what we've done:

  1. SkinStyles for ext.echo.styles.badge are now defined inside the skins. The skin can therefore fully control what the badge looks like prior to and after being clicked.
  2. We introduced a client side hook to allow skins to make modifications to the badge after it is rendered (e.g. add/remove classes). Here's an example of it in use in Vector

On the long term, I suggest that we make Echo progressively enhance any existing Echo badge rather than replacing it with an OOUI button widget.

Since there are issues with Echo in Modern, cologneBlue and Monobook, we should consider T141944 and T288681

For now we'll mostly be focusing on improving the Vector, Vector 2022 and Minerva skins as well as supporting 3rd party skins which are in active development better.

Change 858666 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Move skinStyle from Echo to Minerva

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

Change 858667 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Move skinStyle rules to skin

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

Change 858669 merged by jenkins-bot:

[mediawiki/skins/MonoBook@master] Fix alignment when Echo is clicked

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

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/77fe56a465/w/

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/df7bc7be01/w/

Change 803369 restored by Jdlrobson:

[mediawiki/extensions/Echo@master] Stop rendering the Echo icon - defer to skins

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

Change 803369 abandoned by Jdlrobson:

[mediawiki/extensions/Echo@master] [Minerva desktop] Stop rendering the Echo icon

Reason:

Documented in https://phabricator.wikimedia.org/T343839

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