Page MenuHomePhabricator

User links: Echo icons should match mediawiki.ui.icons in the header
Open, HighPublic3 Estimated Story Points

Description

Due to Echo's behavior of replacing the original icons with OOUI, we cannot easily use core icons for the echo icons. More context on Slack

  • The Echo icons should be styled/overridden to align with other icons in the header.

Context

With the updated user links/menu in the header there will be 5 icon buttons in the header (counting both logged-out and logged-in states):

  1. Hamburger icon
  2. Logged-out user menu
  3. Notifications
  4. Messages
  5. Logged-in user menu
logged-out
image.png (107×1 px, 15 KB)
logged-in
image.png (107×1 px, 16 KB)

Currently we don't have consistency among these icon buttons. Size seems to be consistent, but hover state and touch area are not.

Design details

image.png (77×350 px, 4 KB)

Event Timeline

I believe this is the same bug as https://phabricator.wikimedia.org/T191021 @ovasileva @alexhollender let's talk aobut this in product planning.

The user menu (5), hamburger icon (1) and ellipsis (2) are created using mw-ui-icon and those have the best chance of being consistent with the style guide (and each other).

As for the other icons, they come from Echo extension (3,4) with their own custom styles. Once we are happy with the styling of 1,2 and 5, we should update their implementation.

ovasileva triaged this task as Medium priority.Jun 23 2021, 10:44 AM

I believe this is the same bug as https://phabricator.wikimedia.org/T191021 @ovasileva @alexhollender let's talk aobut this in product planning.

The user menu (5), hamburger icon (1) and ellipsis (2) are created using mw-ui-icon and those have the best chance of being consistent with the style guide (and each other).

As for the other icons, they come from Echo extension (3,4) with their own custom styles. Once we are happy with the styling of 1,2 and 5, we should update their implementation.

Sounds good - moving to needs analysis for now. It does seem to be the same issue as T191021: User links: Standardize `.mw-ui-icon` to overhauled icon canvas size 20x20

@alexhollender out of interest, do any of the icons ad currently rendered reflect the final state we desire? Or are they all flawed in some way?

@alexhollender out of interest, do any of the icons ad currently rendered reflect the final state we desire? Or are they all flawed in some way?

currently, in latest vector, none of them are correct : /
(but in Minerva they are all correct!)

Jdlrobson renamed this task from Standardize icon buttons in header to Echo should use mediawiki.ui.icon in the header.Jun 23 2021, 9:02 PM
Jdlrobson removed Volker_E as the assignee of this task.
Jdlrobson added a project: Notifications.
Jdlrobson updated the task description. (Show Details)

I suggest we use this ticket to focus on getting Echo's icons aligned with Vectors (in whatever form makes sense). I would suggest it makes sense for Echo to register menu items with icon class and for the classes to be added on the Vector side just as we have done with the user menu.

One they are standardized (but incorrect) we can look to do T191021. Will leave notes separately on that ticket.

diff --git a/includes/Hooks.php b/includes/Hooks.php
index 68306a36..7cd6855d 100644
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@@ -162,21 +162,30 @@ class Hooks {
 				unset( $content_navigation['user-menu']['login'] );
 			}
 			// Prefix user link items with associated icon.
-			$user_menu = $content_navigation['user-menu'];
+			$content_navigation['user-menu'] = self::getMenuWithIcons( $content_navigation['user-menu'] );
+			$content_navigation['notifications'] = self::getMenuWithIcons( $content_navigation['notifications'], 'mw-ui-icon' );
+		} else {
+			// Remove user page from personal toolbar since it will be inside the personal menu for logged in
+			// users when the feature flag is disabled.
+			unset( $content_navigation['user-page'] );
+		}
+	}
+
+	private static function getMenuWithIcons( $menu, $iconType = 'mw-ui-icon-before' ) {
 			// Loop through each menu to check/append its link classes.
-			foreach ( $user_menu as $menu_key => $menu_value ) {
+			foreach ( $menu as $menu_key => $menu_value ) {
 				// Check if the menu has an icon key (provided by extensions). If not, get the icon from the icon map.
 				$icon_name = array_key_exists( 'icon', $menu_value )
 					? $menu_value['icon']
 					: self::getIconFromKey( $menu_key );
 				// Set the default menu icon classes.
-				$menu_icon_classes = [ 'mw-ui-icon', 'mw-ui-icon-before', 'mw-ui-icon-wikimedia-' . $icon_name ];
+				$menu_icon_classes = [ 'mw-ui-icon', $iconType, 'mw-ui-icon-wikimedia-' . $icon_name ];
 
 				// Add the link-class key to the menu and its relevant classes if it doesn't exist.
 				if ( !( array_key_exists( 'class', $menu_value )
 					|| array_key_exists( 'link-class', $menu_value ) )
 				) {
-					$content_navigation['user-menu'][$menu_key]['link-class'] = $menu_icon_classes;
+					$menu[$menu_key]['link-class'] = $menu_icon_classes;
 				} else {
 					// The link-class or class keys do exist, so append the icon classes to them.
 					foreach ( $menu_value as $link_key => $link_value ) {
@@ -185,7 +194,7 @@ class Hooks {
 							case 'class':
 							case 'link-class':
 								$prior_link_classes = is_array( $link_value ) ? $link_value : [ $link_value ];
-								$content_navigation['user-menu'][$menu_key][$link_key] = array_merge(
+								$menu[$menu_key][$link_key] = array_merge(
 									$prior_link_classes,
 									$menu_icon_classes
 								);
@@ -194,13 +203,8 @@ class Hooks {
 					}
 				}
 			}
-		} else {
-			// Remove user page from personal toolbar since it will be inside the personal menu for logged in
-			// users when the feature flag is disabled.
-			unset( $content_navigation['user-page'] );
-		}
+			return $menu;
 	}
-
 	/**
 	 * Upgrades Vector's watch action to a watchstar.
 	 *
ovasileva renamed this task from Echo should use mediawiki.ui.icon in the header to User links: Echo should use mediawiki.ui.icon in the header.Mon, Jun 28, 5:25 PM

Un-licking this one. If working on this, please check out T191021 first. I believe we need to do this to get the correct hover and focus states which are missing.

Nope. You can do this before or after. You'll likely need to test this on the current version as well as a version with https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/702456 and the core depends-on patch checked out.

@Jdlrobson

I suggest we use this ticket to focus on getting Echo's icons aligned with Vectors (in whatever form makes sense). I would suggest it makes sense for Echo to register menu items with icon class and for the classes to be added on the Vector side just as we have done with the user menu.

I was trying this approach, and I think it might not work because the "notifications" menu data is still empty inside Hooks::updateUserLinksItems

Nevermind, Jon helped figure out I had an issue with my LocalSettings

ovasileva raised the priority of this task from Medium to High.Thu, Jul 15, 5:41 PM

Change 704862 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/extensions/Echo@master] Add icon property to echo link data for modern Vector

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

Change 704870 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Update echo badges to use mw-ui-icon implementation in modern Vector

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

Change 704862 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Add icon property to echo link data for modern Vector

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

Change 705411 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/extensions/Echo@master] Update vector badge styles

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

bwang removed bwang as the assignee of this task.Tue, Jul 20, 10:12 PM
bwang added a subscriber: bwang.
Jdlrobson lowered the priority of this task from High to Medium.Wed, Jul 21, 4:21 PM

Adjusting priority compared to other things in code review.

bwang renamed this task from User links: Echo should use mediawiki.ui.icon in the header to User links: Echo icons should match mediawiki.ui.icons in the header.Wed, Jul 21, 4:47 PM
bwang updated the task description. (Show Details)

Change 705411 had a related patch set uploaded (by Jdlrobson; author: Bernard Wang):

[mediawiki/extensions/Echo@master] Update vector badge styles

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

Jdlrobson raised the priority of this task from Medium to High.Thu, Jul 22, 12:13 AM

Padding is off a little and needs more work.

currently:

Screen Shot 2021-07-21 at 3.54.15 PM.png (90×176 px, 5 KB)

expected:

Screen Shot 2021-07-21 at 3.55.24 PM.png (146×166 px, 8 KB)

Bumping to high priority now the other issues have been resolved given this is now our most important :)

Change 705411 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Update vector badge styles

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

@Jdlrobson @bwang

  • we're missing padding between the notification icon and the username, but maybe that's happening in T285786?

image.png (96×297 px, 5 KB)

  • the hover color is too light (should match Minerva, see comment here T191021#7231088)

not sure if I should put this in Needs more work or if y'all want to handle those fixes elsewhere

the hover color is too light (should match Minerva, see comment here T191021#7231088)

It does match Minerva? Minerva uses background-color: rgba(0,0,0,0.03); which this is also using. Perhaps we need to change that default? :) What is the correct background color?

the hover color is too light (should match Minerva, see comment here T191021#7231088)

It does match Minerva? Minerva uses background-color: rgba(0,0,0,0.03); which this is also using. Perhaps we need to change that default? :) What is the correct background color?

it's slightly lighter than Minerva (which I assume means it will be slightly lighter than the hamburger and user menu icon once those are updated?):

image.png (70×127 px, 1 KB)

I think it's because those two icons already have this opacity: 0.51 rule, which I believe is effective whenever they don't have new notifications inside of them?

image.png (748×484 px, 128 KB)

Ah yes.. the opacity would definitely explain that. Thanks for expanding. @bwang perhaps we could set the opacity explicitly in the skin style for modern Vector or restrict this style in some way?

NOTE: the opacity only applies when you've cleared all notifications e.g. no number is present in a box.

@Jdlrobson Yeah I could put up a follow up patch for Echo. I was thinking it might be easier to disable the opacity entirely for user links, and just set the color explicitly, what do you think?

@alexhollender Do you want to preserve the existing behavior for Echo, where the icon is lighter when everything is read, or do you want the echo icons to behave similarly to the user menu icon? And if the latter, do you know exactly what color it should be? I believe right now it's #202122

@alexhollender Do you want to preserve the existing behavior for Echo, where the icon is lighter when everything is read, or do you want the echo icons to behave similarly to the user menu icon? And if the latter, do you know exactly what color it should be? I believe right now it's #202122

it would be fine to simplify things here and remove that extra layer of opacity, since they get little numbers on them anyways when you have notifications. I need to figure out what the right color is for all of these icons, for now just go with whatever we're doing for the user menu icon. I will file a separate task for color consistency at some point (since, for example, the hamburger icon doesn't match the user menu icon, and even the little down arrow next to the user icon doesn't match lol)

Change 707389 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/extensions/Echo@master] Override opacity styles for all read badges for vector user links

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

@alexhollender Ok, if that's the case I'll just remove the opacity, which will look like this: https://jmp.sh/hpK2Jte
I made a mistake earlier, both echo icons and the user menu icons are actually just black #000, so I'll just leave them as be until that other color task

Change 707389 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Override opacity styles for all read badges for vector user links

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

Jdlrobson added a subscriber: nray.
we're missing padding between the notification icon and the username, but maybe that's happening in T285786?

@alexhollender let's track the positioning of the that in T285786 (cc @nray
With that in mind, this should be ready for one last design review. Move to sign off if good.

@alexhollender Ok, if that's the case I'll just remove the opacity, which will look like this: https://jmp.sh/hpK2Jte
I made a mistake earlier, both echo icons and the user menu icons are actually just black #000, so I'll just leave them as be until that other color task

looks good

we're missing padding between the notification icon and the username, but maybe that's happening in T285786?

@alexhollender let's track the positioning of the that in T285786 (cc @nray
With that in mind, this should be ready for one last design review. Move to sign off if good.

ok cool