In T216418#5125379, @alexhollender wrote:

@nray well that height was set based on the assumption of the minimum number of items that would appear in that menu. Do you know if the case in the screenshot you posted above is a real one, or something that would only occur on staging. I also notice that there is no history icon in that screenshot, which seems peculiar.

In T217298#5120067, @nray wrote:

@ovasileva I don't think this has anything to do with the changes here, but I noticed that when I click on the blue "Okay" button on BetaOptinPanel (pictured below) AND I have AMC enabled, it will turn off AMC mode. I'm assuming that is incorrect behavior; should I make a ticket for this?

In T216418#5098537, @alexhollender wrote:

Following up on the remaining open topics:

==== 1. Menu scrolling behavior
Regarding when there is not enough vertical space to display the entire menu, either because the phone is in landscape mode, or has a shorter screen.

For JS users we will do the following:

set a max-height on the menu, taking into account the offset from top of screen	allow the menu to scroll internally	set a min-height of 220px so that the menu never gets too short

For non-JS users we will do the following:

Not set a max-height	defer to scrolling the page to access the additional menu items

==== 2. Menu item icons
I've updated the task description to indicate which OOUI icon to use for each menu item

@ovasileva I don't think this has anything to do with the changes here, but I noticed that when I click on the blue "Okay" button on BetaOptinPanel (pictured below) AND I have AMC enabled, it will turn off AMC mode. I'm assuming that is incorrect behavior; should I make a ticket for this?

In T220751#5111847, @ovasileva wrote:

pulling this into the sprint as it's actively being worked on. @nray - should we put it in QA?

In T220159#5110028, @Edtadros wrote:

@nray This needs QA steps.

In T218627#5110077, @Edtadros wrote:

=== Test Result

Status: Need More Info
OS: macOS Mojave
Browser: Chrome
Device: MBP

@nray I tried 1a, 1b, 3a, 3b about 10 times each and only saw amc:false once. Additionally when the instructions say "no event shows up" does this mean no event with the amc:false parameter, or no event at all. I get 3 beacon events every time for 1a and 1b, and two beacon events for 3a and 3b.

In the likely event I'm doing it wrong, after I click on the hamburger menu, I right click on Random and open that in a new window. I have Chrome to open dev tools automatically so it captures the traffic in the newly opened window. Given the randomness of 1a, 1b, 3a, and 3b, what sample size would you expect to see a 50/50 breakdown?

@Jdlrobson thanks for listing the QA steps, I'm not sure that Scenario 2b is possible (nor that we want it to be possible) according to the AC:

@Jdlrobson I'm trying to put QA instructions on this, but I'm not sure exactly what we should be QAing. How in-depth with this do we want to go?

Thank you @phuedx, that is a very helpful diagram explanation. One more question I had while looking at: If we only rely on the pagehide event, isn't it possible to miss the path that goes from HIDDEN => freeze => FROZEN? It looks like hidden can go directly to freeze in that diagram; should we care about that path?

In T216198#5058762, @nray wrote:

One more patch that I'd really like to get out of the way before moving this do QA/Ready for sign off:

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/498998/

One more patch that I'd really like to get out of the way before moving this do QA/Ready for sign off:

In T211162#5018909, @Jdlrobson wrote:

@nray while you are looking at the ImageOverlay could you take a look at if your changes in T216198 can or could fix this?

Notifications filter

No longer see the problematic text reported in the description.

@Jdlrobson language searcher code coverage patch is at https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/493272/