Page MenuHomePhabricator

Repurpose BetaOptinPanel as a Panel
Closed, ResolvedPublic5 Story Points

Description

Panels, Drawers and CtaDrawers.. oh my. These have been causing us some anxiety, so we need an opportunity to get more familiar with patterns that will help us use them better.

The BetaOptinPanel is a good place to start given the fact it is disabled in production (at time of writing) and simple in its responsibilities. It lives inside mobile.init

For those of you who don't know what a BetaOptInPanel is... it displays on stable and asks you to opt into beta you can enable it using:

$wgMFExperiments['betaoptin'] = array(
                "name" => "betaoptin",
                'buckets' => [
                        'control' => 0,
                        'A' => 1,
                ],
                'enabled' => true,
);

And visiting a page in the Minerva skin using an incognito window.

Acceptance criteria

  • BetaOptinPanel should be a Panel rather than extend a Panel.
  • Events should be passed in as parameters rather than by extending and overriding class properties
  • The new BetaOptinPanel should not make use of templatePartials - instead it should append a form which contains Button's inside the newly created Panel.
  • The template associated with BetaOptInPanel should be removed. (template still seems useful)

Developer notes

The existing template for BetaOptInPanel looks like:

<form class="message content" action="{{postUrl}}" method="POST">
	<p>{{text}}</p>
	<input type="hidden" name="enableBeta" value="true">
	<input type="hidden" name="token" value="{{editToken}}">
	{{#buttons}}{{>button}}{{/buttons}}
</form>

A Drawer is a Panel (and thus a CtaDrawer is too), nothing uses Drawer directly (right now), so be aware of how your changes impact CtaDrawers or ReferenceDrawer

There was an interesting conversation around panel.less that is worth considering as there may be an opportunity to clean code up during the implementation of this task:
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/493757/3/resources/mobile.startup/panel.less,unified

QA steps

  • Verify the beta optin panel is still functional (on staging)
  • When anonymous click the watchstar, verify a drawer shows with login/sign in buttons that can be dismissed
  • Find a red link (A link to a page that doesn't exist) and verify that a drawer shows when clicked
  • Find a reference and verify that a black references drawer displays when you click the reference

QA Results

ACStatusDetails
1T217298#5110109
2T217298#5110109
3T217298#5110109 Please see note on anomoly
4T217298#5110109

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterBetaOptInPanel should be removed when "cancel" is clicked
mediawiki/extensions/MobileFrontend : masterInline the BetaOptInPanel template and drop partial usage
mediawiki/extensions/MobileFrontend : masterHygiene: simplify BetaOptInPanel
mediawiki/extensions/MobileFrontend : masterFix: BetaOptInPanel centering on large screens
mediawiki/extensions/MobileFrontend : masterHygiene: rename BetaOptinPanel
mediawiki/extensions/MobileFrontend : masterHygiene: replace BetaOptinPanel parent

Event Timeline

phuedx removed a subscriber: phuedx.Feb 28 2019, 10:58 AM
Jdlrobson set the point value for this task to 5.Mar 5 2019, 5:22 PM

@ovasileva FYI this came up during grooming today. Apparently this component would allow us to present the type of contextual AMC hook that we've discussed, such as:

Jdlrobson updated the task description. (Show Details)Mar 5 2019, 10:48 PM
Niedzielski updated the task description. (Show Details)Mar 7 2019, 3:21 PM
Jdlrobson raised the priority of this task from High to Needs Triage.Mar 8 2019, 8:03 PM
Jdlrobson triaged this task as Normal priority.Mar 8 2019, 9:35 PM

Change 497943 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: replace BetaOptinPanel parent

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

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptMar 20 2019, 10:54 PM

Change 497943 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Hygiene: replace BetaOptinPanel parent

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

Change 498194 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename BetaOptinPanel

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

Change 498195 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Fix: BetaOptInPanel centering on large screens

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

Change 498194 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename BetaOptinPanel

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

Change 498226 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: replace BetaOptInPanel template partials

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

@Jdlrobson, I'm assigning this task task to you since you have some open discussion on the patches in flight and I want to give you a chance to ok / nay them instead of the patches just getting steamrolled into master. I've followed up on comments and set expectations with the team that you may not have time to respond until you return from Volker's World™ 👍

Change 498195 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix: BetaOptInPanel centering on large screens

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

Blocked until we can talk about it

I've moved this to QA per my understanding of super-happy-dev-time discussion. @Jdlrobson, please punt back if you disagree.

Jdlrobson updated the task description. (Show Details)Apr 9 2019, 7:49 PM

Change 498226 abandoned by Niedzielski:
Hygiene: simplify BetaOptInPanel

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

Change 502596 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] BetaOptInPanel should be removed when "cancel" is clicked

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

Change 502597 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Inline the BetaOptInPanel template and drop partial usage

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

As I started to write QA steps, I realised that this is broken. When I click the "cancel" button, the panel is meant to be destroyed, but this is no longer the case. Patch is up and blocks moving this to QA: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/502596 BetaOptInPanel should be removed when "cancel" is clicked

Reading through the acceptance criteria, one remains - the removal of the Button templatePartial. If we're not going to resolve this now, I suggest we move this back to the backlog or cut out a new task. The way we use Button.options is problematic for performance reasons and will likely causes us problems as we inline templates. Can cut a new bug for that if necessary:
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/502597 Inline the BetaOptInPanel template and drop partial usage

Change 502596 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] BetaOptInPanel should be removed when "cancel" is clicked

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

Change 502597 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Inline the BetaOptInPanel template and drop partial usage

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

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome
Device: MBP (iPhoneX emulated on Chrome devtools)

Test Artifact(s):

QA steps

✅ Verify the beta optin panel is still functional (on staging)

✅ When anonymous click the watchstar, verify a drawer shows with login/sign in buttons that can be dismissed

✅ Find a red link (A link to a page that doesn't exist) and verify that a drawer shows when clicked

@Niedzielski For some reason on this page using the Chrome devtools emulator, the drawer overlay appeared incorrectly. I verified that on an actual mobile device it appears normally. So I'm passing it. I'm including it here for your information. This could be a bug.

✅ Find a reference and verify that a black references drawer displays when you click the reference

nray claimed this task.Apr 15 2019, 6:36 PM
nray updated the task description. (Show Details)Apr 17 2019, 6:41 PM
nray added a comment.Apr 17 2019, 6:54 PM

@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?

nray closed this task as Resolved.Apr 18 2019, 5:16 PM
nray added a comment.Apr 18 2019, 5:37 PM

@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?

I made a ticket for this at T221395 in case it is worthy of one. @ovasileva feel free to delete this if not. BetaOptinPanel isn't turned on in production so this might not be relevant.

@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?

I made a ticket for this at T221395 in case it is worthy of one. @ovasileva feel free to delete this if not. BetaOptinPanel isn't turned on in production so this might not be relevant.

Hey @nray - thank you, a new task sounds good. Sorry for not replying earlier!