Page MenuHomePhabricator

[EPIC] AMC Navigation - user menu
Closed, ResolvedPublic5 Story Points

Description

User Story

  • As an advanced user, I want all the navigational elements pertaining to my account to be collected in one place so that I can get to them quicker
  • As an advanced user, I want quick access to the following pages, so that I don’t have to bookmark them: my sandbox, my contributions, my talk page, watchlist, settings

Background

As a part of the new navigation for AMC, we would like to separate all user-based actions into a separate menu

Acceptance criteria

The user menu will contain the following links:

  • Username (navigates to user’s user page)
  • Talk (navigates to user’s talk page)
  • Sandbox
  • Watchlist (navigates to user’s mobile watchlist)
  • Contributions
  • Log out
NOTE: these links (username, watchlist, contributions, log out) will be removed from the main menu
  • There should NOT be any duplication between the main menu and user menu links

Design

Design notes

  • currently there is ~72px of space between the Notification and Search icon for logged-in users. With the addition of the User icon we will need to decrease the spacing between the icons to ~50px, while maintaining the same amount of spacing to the right of the right-most icon (User icon).

Icons

ltrrtl
contributions
sandbox(same)
talk

QA

All testing may be performed on the beta cluster. E.g., https://en.m.wikipedia.beta.wmflabs.org/wiki/Dog.

  • Some of the links from the main menu have moved to the user menu and a couple new ones were added. For _moved_ links, they should point to the same location as those in the main menu.
  • There should be no redundancy between the menus. I.e., if a link appears in one menu, it should not appear in the other.
  • No links have been removed. I.e., if a link is no longer in the main menu, it should appear in the user menu.
  • Some links have been added. They should be valid and match those found in the Vector desktop sidebar.
  • Some of the precarious toolbar styling was updated to make space for the new user menu icon. Be on the look out for visual regressions in the toolbar itself as well as overlays (notifications, search) and the main menu at different device screen breakpoint sizes (desktop, tablet, phone).
  • The page action overflow menu was significantly refactored into a reusable component. Keep your eyes peeled for visual and functional regressions.
  • Desktop Minerva (e.g., https://en.m.wikipedia.beta.wmflabs.org/w/index.php?title=Dog&useskin=minerva) does not have a default mode even for logged out users.
  • TODO: GrowthExperiments instructions here. Growth is taking care of it, no need to QA
  • For all of the above: AMC and non-AMC mobile Minerva, and desktop Minerva (AMC is always on), each configuration should be tested in JavaScript and non-JavaScript mode.

Related Objects

Event Timeline

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

Change 521558 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] [UI] reduce icon sizes to 54px

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

alexhollender added a comment.EditedJul 11 2019, 6:27 PM

@Niedzielski as discussed in Slack, here is the updated icon:

and the mockup for reference:

Change 522229 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] [dev] make interface for overriding menu profile URL

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

Change 522230 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] [dev] split AuthMenuEntry

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

Current task status:

No OOUI-style callouts (triangle), like Echo?

Where we don't use callouts for popups are dropdown- style elements, where the handle (element which opens the dropdown) gets some focus styling.

In this design there is no visual confirmation that the user clicked on the correct icon, which on a small touch device with a large thumb is not always a given. If there multiple menus next to each other, the user could open the wrong menu due to a miss-hit, but still think they hit the right one.

@Esanders thanks for pointing this out. I see visual feedback when I tap on the menu icons (on iOS and Android):

upon tappingmenu open

As you suggest, I agree it'd be a great idea to persist that styling while the menu is open, cc @Jdrewniak. I will create a task for that.

Change 524927 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] [dev] Replace menu entry inheritance with functions

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

I've rebased all the patches and have been addressing feedback. The following patches are awaiting review:

https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/522229/
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/522230/
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/524927/

The following patches still have feedback I'm addressing:

https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/519126/
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/521558/

The latest changes are available for evaluation here:

https://readers-web-stephen.wmflabs.org/wiki/Foobarbaz

@alexhollender, the user avatar outline seems a little thick to me. Do you think it should be finer to be more consistent with the fine lines of the bell and page info icons? The bell also has varied thickness:

@alexhollender, the user avatar outline seems a little thick to me.

This was due to a bad rebase. I'll follow up.

Change 522229 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] [dev] make interface for overriding menu profile URL

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

Change 522230 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] [dev] split AuthMenuEntry

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

Change 524927 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] [dev] Replace menu entry inheritance with functions

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

This was due to a bad rebase. I'll follow up.

This still looks funny to me. @alexhollender please make sure everything looks ok to you in AMC and default modes.

The latest changes are available for evaluation here:

https://readers-web-stephen.wmflabs.org/wiki/Foobarbaz

Thanks for the reviews @Jdlrobson, @pmiazga, @nray. First three patches are merged. All feedback is addressed on the two remaining:
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/519126/
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/521558/

This still looks funny to me. @alexhollender please make sure everything looks ok to you in AMC and default modes.

Yea the user icon is a bit large, though I'm hoping things will sort themselves out once we do T224070. I'm happy to move forward with this as is.

Process note: in retrospect I think I should've kept the scope here down. Adding the menu was a big enough task, without having the additional work of swapping out icons, and messing with spacing. I should've just created separate tasks for that work. Seems like as a general rule smaller tasks are better than large ones.

Change 519126 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] [UI] [new] add user menu

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

Okay @alexhollender judging on https://phabricator.wikimedia.org/T214540#5359772 let's move forward with design review as is and open some new tasks for the remaining work. Staging has the latest and greatest for you to review: https://readingwebstaging.wmflabs.org/

👍👍looks great. for screenshots see T224070#5363828

@Niedzielski thoughts on whether or not we want to send this task to QA?

I'll write QA steps tomorrow. Thanks @alexhollender, @Jdlrobson, @pmiazga <3

Change 525515 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Follow-up: User menu improvements

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

Change 525518 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Follow-up: clean up old Advanced Menu builder and add hook support

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

Change 525518 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Follow-up: clean up old Advanced Menu builder and add hook support

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

@Edtadros, I've updated the description to include testing vectors. I'm guessing this is our #1 priority or somewhere near the top. I've tried to keep it brief but comprehensive. If I've failed to elaborate sufficiently or you have questions, please ping me! GrowthExperiments instructions forthcoming.

Change 525515 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Follow-up: User menu improvements

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

Change 525579 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/GrowthExperiments@master] Support new AMC user menu

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

Edtadros added a subscriber: Edtadros.

QA

All testing may be performed on the beta cluster. E.g., https://en.m.wikipedia.beta.wmflabs.org/wiki/Dog.
✅ AC1: Some of the links from the main menu have moved to the user menu and a couple new ones were added. For _moved_ links, they should point to the same location as those in the main menu.

✅ AC2: There should be no redundancy between the menus. I.e., if a link appears in one menu, it should not appear in the other.
See AC1

✅ AC3: No links have been removed. I.e., if a link is no longer in the main menu, it should appear in the user menu.
See AC1

❓ AC4 Some links have been added. They should be valid and match those found in the Vector desktop sidebar.
@Niedzielski can you please provide the specific I should be looking for from the Vector sidebar to the User Menu.

❌ AC5 Some of the precarious toolbar styling was updated to make space for the new user menu icon. Be on the look out for visual regressions in the toolbar itself as well as overlays (notifications, search) and the main menu at different device screen breakpoint sizes (desktop, tablet, phone).
It doesn't appear that the notifications are being counted correctly. It's not shown below, but later when I reloaded the page the notifications were accurate.

✅ AC6: The page action overflow menu was significantly refactored into a reusable component. Keep your eyes peeled for visual and functional regressions.

❓ AC7: Desktop Minerva (e.g., https://en.m.wikipedia.beta.wmflabs.org/w/index.php?title=Dog&useskin=minerva) does not have a default mode even for logged out users.
@Niedzielski, I'm unsure how to test this.

⬜ AC8: TODO: GrowthExperiments instructions here.

❓AC9: For all of the above: AMC and non-AMC mobile Minerva, and desktop Minerva (AMC is always on), each configuration should be tested in JavaScript and non-JavaScript mode.
@Niedzielski just to clarify, AC1-8 need to be tested for all of the following permutations:
AMC-ON, JS On
AMC-ON, JS Off
AMC-Off, JS On
AMC-Off, JS Off
Desktop, JS On
Desktop, JS Off

Also for desktop, I am assuming I would test the following URL:
https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Dog&useskin=minerva

Please confirm.

Thanks @Edtadros!

AC4

I was wrong about this. I guess the Sandbox is the only new link since contributions was already in the AMC main menu. This link is actually in top of Vector, not the sidebar like I said originally. Sorry for the confusion.

AC5

Is this T220979? If not, please open a new ticket as this issue is unlikely to be related to the user menu.

AC7

Sorry, this mode can be tested by adding ?useskin=minerva to the URL like https://en.wikipedia.beta.wmflabs.org/wiki/Dog?useskin=minerva. Let me know if this isn't working for you.

AC9

These are these are the modes:

  • Logged in, AMC-ON, JS On
  • Logged in, AMC-ON, JS Off
  • Logged in, AMC-Off, JS On
  • Logged in, AMC-Off, JS Off
  • Logged out, JS On
  • Logged out, JS Off
  • Desktop, logged in, JS On <-- Desktop Minerva only (via the useskin URL parameter)
  • Desktop, logged in, JS Off <-- Desktop Minerva only (via the useskin URL parameter)
  • Desktop, logged out, JS On <-- Desktop Minerva only (via the useskin URL parameter)
  • Desktop, logged out, JS Off <-- Desktop Minerva only (via the useskin URL parameter)

Also for desktop, I am assuming I would test the following URL:
https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Dog&useskin=minerva

That's correct.

If anything's unclear, please let me know.

Change 525579 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Support new AMC user menu

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

Test Result

Status: ⬜ In Progress
OS: macOS Mojave
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA

All testing may be performed on the beta cluster. E.g., https://en.m.wikipedia.beta.wmflabs.org/wiki/Dog.
Testing should be on all of the following modes:
A - Logged in, AMC-ON, JS On
B - Logged in, AMC-ON, JS Off
C - Logged in, AMC-Off, JS On
D - Logged in, AMC-Off, JS Off
E - Logged out, JS On
F - Logged out, JS Off
G - Desktop, logged in, JS On <-- Desktop Minerva only (via the useskin URL parameter)
H - Desktop, logged in, JS Off <-- Desktop Minerva only (via the useskin URL parameter)
I - Desktop, logged out, JS On <-- Desktop Minerva only (via the useskin URL parameter)
J - Desktop, logged out, JS Off <-- Desktop Minerva only (via the useskin URL parameter)

⬜ AC1: Some of the links from the main menu have moved to the user menu and a couple of new ones were added. For _moved_ links, they should point to the same location as those in the main menu.

ABCDEFGHIJ
No user menu
No user menuNo user menu
Icons do not appear.
Icons do not appear.

⬜ AC2: There should be no redundancy between the menus. I.e., if a link appears in one menu, it should not appear in the other.

ABCDEFGHIJ
See AC1See AC1See AC1See AC1See AC1See AC1See AC1See AC1See AC1See AC1

⬜ AC3: No links have been removed. I.e., if a link is no longer in the main menu, it should appear in the user menu.

ABCDEFGHIJ
See AC1See AC1See AC1See AC1See AC1See AC1See AC1See AC1See AC1See AC1

⬜ AC4 Verify the sandbox link appears in the user menu. (Per T214540#5366529)

ABCDEFGHIJ
See AC1See AC1See AC1See AC1See AC1See AC1See AC1See AC1See AC1See AC1

⬜ AC5 Some of the precarious toolbar styling was updated to make space for the new user menu icon. Be on the look out for visual regressions in the toolbar itself as well as overlays (notifications, search) and the main menu at different device screen breakpoint sizes (desktop, tablet, phone).
There don't appear to be any visual regressions, however, it doesn't appear that the notifications are being counted correctly. It's not shown below, but later when I reloaded the page the notifications were accurate. In other instances, reloading the page would give a higher number of notifications. In the examples below only one talk page action was taking which should have only resulted in one notification.

ABCDEFGHIJ
See AC1
See AC1See AC1

⬜ AC6: The page action overflow menu was significantly refactored into a reusable component. Keep your eyes peeled for visual and functional regressions.

ABCDEFGHIJ
No Menu
No MenuNo MenuNo MenuNo MenuNo MenuNo Menu

⬜ AC7: Desktop Minerva (e.g., https://en.m.wikipedia.beta.wmflabs.org/w/index.php?title=Dog&useskin=minerva) does not have a default mode even for logged out users.

ABCDEFGHIJ

⬜ AC8: TODO: GrowthExperiments instructions here.

ABCDEFGHIJ

@Niedzielski I went through the test for every mode. With regards to AC7, I'm still not sure what I'm supposed to test. Once you have steps for AC8 I can also conduct the AC7 tests. With regards to AC5, I will open a ticket for the notification issue. It may make sense to strip out AC5, 7, and 8 to a new task. The remaining tests only show one issue, unless the instances where I show no menu should have actually had a menu. Let me know your thoughts.

I went through the test for every mode.

Thank you @Edtadros. Outstanding work as usual.

AC8: TODO: GrowthExperiments instructions here.

@pmiazga, can you add QA instructions once your patch is merged?

AC1 H, J

This is a legit bug. I can repro. I'll investigate Monday.

AC5
There don't appear to be any visual regressions, however, it doesn't appear that the notifications are being counted correctly. It's not shown below, but later when I reloaded the page the notifications were accurate. In other instances, reloading the page would give a higher number of notifications. In the examples below only one talk page action was taking which should have only resulted in one notification.
With regards to AC5, I will open a ticket for the notification issue.

Sounds good. I think these are preexisting issues in the Echo Notifications extension.

With regards to AC7, I'm still not sure what I'm supposed to test.

AC7 is covered by testing these modes:

  • Desktop, logged in, JS On <-- Desktop Minerva only (via the useskin URL parameter)
  • Desktop, logged in, JS Off <-- Desktop Minerva only (via the useskin URL parameter)
  • Desktop, logged out, JS On <-- Desktop Minerva only (via the useskin URL parameter)
  • Desktop, logged out, JS Off <-- Desktop Minerva only (via the useskin URL parameter)

It looks like you've already tested it in your above matrix.

The remaining tests only show one issue, unless the instances where I show no menu should have actually had a menu.

@Jdlrobson, @pmiazga, what is the intended behavior of the page actions overflow menu in Minerva desktop mode? Is it expected to appear or be hidden? I think it's supposed to always be shown in Minerva desktop mode and that this is a bug but I'm pretty confused about this rare configuration.

It may make sense to strip out AC5, 7, and 8 to a new task.

I think the only thing that needs a new task is the bug(s) you found for the notification count in AC5.

Change 526158 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] [fix] [AMC] load main menu icons on desktop Minerva

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

Change 521558 abandoned by Niedzielski:
[UI] reduce toolbar and icon sizes, update user icon

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

Change 526158 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] [fix] [AMC] load main menu icons on desktop Minerva

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

AC1 H, J

This is a legit bug. I can repro. I'll investigate Monday.

Fixed and available on BC.

pmiazga updated the task description. (Show Details)Jul 29 2019, 8:43 PM
pmiazga updated the task description. (Show Details)
pmiazga added a subscriber: Catrope.EditedJul 29 2019, 8:46 PM

I removed the TODO: GrowthExperiments instructions here. from the QA steps. Growth team fixed the issue in rEGRE74537902fa59: Support new AMC user menu and from my knowledge they properly QAed the change.
/cc @Catrope

@Jdlrobson, @pmiazga, what is the intended behavior of the page actions overflow menu in Minerva desktop mode

I think we resolved this?

I think the only thing that needs a new task is the bug(s) you found for the notification count in AC5.

Sounds good to me (although the beta cluster has a few issues with Echo already and it may be a known problem)

what's left? Can this go into sign off now?

@Jdlrobson, @pmiazga, what is the intended behavior of the page actions overflow menu in Minerva desktop mode

I think we resolved this?

I'm still unclear what the expected behavior is. Should we show the page actions overflow menu or not in Minerva desktop? Here's the current behavior:

I'm still unclear what the expected behavior is. Should we show the page actions overflow menu or not in Minerva desktop? Here's the current behavior:

I've cut a new bug for that -> T229295 It's not important for this task.

Edtadros reassigned this task from Edtadros to phuedx.Jul 31 2019, 2:38 AM

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA

All testing may be performed on the beta cluster. E.g., https://en.m.wikipedia.beta.wmflabs.org/wiki/Dog.
Testing should be on all of the following modes:
A - Logged in, AMC-ON, JS On
B - Logged in, AMC-ON, JS Off
C - Logged in, AMC-Off, JS On
D - Logged in, AMC-Off, JS Off
E - Logged out, JS On
F - Logged out, JS Off
G - Desktop, logged in, JS On <-- Desktop Minerva only (via the useskin URL parameter)
H - Desktop, logged in, JS Off <-- Desktop Minerva only (via the useskin URL parameter)
I - Desktop, logged out, JS On <-- Desktop Minerva only (via the useskin URL parameter)
J - Desktop, logged out, JS Off <-- Desktop Minerva only (via the useskin URL parameter)

✅ AC5 Some of the precarious toolbar styling was updated to make space for the new user menu icon. Be on the look out for visual regressions in the toolbar itself as well as overlays (notifications, search) and the main menu at different device screen breakpoint sizes (desktop, tablet, phone).
There don't appear to be any visual regressions.

ABCDEFGHIJ
See AC1 T214540#5370993See AC1 T214540#5370993See AC1 T214540#5370993See AC1 T214540#5370993See AC1 T214540#5370993See AC1 T214540#5370993See AC1 T214540#5370993See AC1 T214540#5370993See AC1 T214540#5370993See AC1 T214540#5370993
phuedx updated the task description. (Show Details)Jul 31 2019, 10:59 AM

As you suggest, I agree it'd be a great idea to persist that styling while the menu is open, cc @Jdrewniak. I will create a task for that.

@alexhollender: Did you create that task?

phuedx closed this task as Resolved.Jul 31 2019, 12:35 PM

This looks great to me 💪 Excellent work everyone!

Also, I'm pretty sure that T229399: [Bug] [AMC] 1px gap at bottom of page actions without download icon (due to font size usage) wasn't introduced while the team was working on this task so…

As you suggest, I agree it'd be a great idea to persist that styling while the menu is open, cc @Jdrewniak. I will create a task for that.

@alexhollender: Did you create that task?

I've created a to-do for myself. No task yet, but I will soon.