-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FXVPN-10: onboarding flow #142
Changes from 8 commits
e8a40da
677e233
afa3aaa
20a6ef7
a191fbd
dae0fe4
6e69e75
5b2f422
825cf78
1e90c4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/* This Source Code Form is subject to the terms of the Mozilla Public | ||
* License, v. 2.0. If a copy of the MPL was not distributed with this | ||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
// @ts-check | ||
|
||
import { Component } from "./component.js"; | ||
// import { VPNController, VPNState } from "../vpncontroller/index.js"; | ||
import { property } from "../shared/property.js"; | ||
import { PropertyType } from "./../shared/ipc.js"; | ||
import { fromStorage, putIntoStorage } from "./vpncontroller/vpncontroller.js"; | ||
|
||
const ONBOARDING_KEY = "mozillaVpnOnboarding"; | ||
const FIRST_PAGE = 1; | ||
export const NUMBER_OF_ONBOARDING_PAGES = 3; | ||
const FIRST_UNUSED_PAGE = NUMBER_OF_ONBOARDING_PAGES + 1; | ||
|
||
/** | ||
* Handles onboarding. | ||
* | ||
*/ | ||
export class OnboardingController extends Component { | ||
static properties = { | ||
nextOnboardingPage: PropertyType.Function, | ||
finishOnboarding: PropertyType.Function, | ||
currentOnboardingPage: PropertyType.Bindable, | ||
}; | ||
|
||
/** | ||
* | ||
* @param {*} receiver | ||
*/ | ||
constructor(receiver) { | ||
super(receiver); | ||
this.#mCurrentOnboardingPage = property(FIRST_PAGE); | ||
} | ||
|
||
async init() { | ||
this.#mCurrentOnboardingPage.value = await fromStorage( | ||
browser.storage.local, | ||
ONBOARDING_KEY, | ||
FIRST_PAGE | ||
); | ||
} | ||
|
||
get currentOnboardingPage() { | ||
return this.#mCurrentOnboardingPage.readOnly; | ||
} | ||
|
||
nextOnboardingPage() { | ||
this.#mCurrentOnboardingPage.set(this.#mCurrentOnboardingPage.value + 1); | ||
} | ||
|
||
finishOnboarding() { | ||
this.#mCurrentOnboardingPage.set(FIRST_UNUSED_PAGE); | ||
putIntoStorage(FIRST_UNUSED_PAGE, browser.storage.local, ONBOARDING_KEY); | ||
} | ||
|
||
#mCurrentOnboardingPage = property(FIRST_PAGE); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ | |
import { html, render } from "../vendor/lit-all.min.js"; | ||
import { MessageScreen } from "./message-screen.js"; | ||
import { tr } from "../shared/i18n.js"; | ||
import { onboardingController } from "../ui/browserAction/backend.js"; | ||
import { NUMBER_OF_ONBOARDING_PAGES } from "../background/onboarding.js"; | ||
|
||
const open = (url) => { | ||
browser.tabs.create({ | ||
|
@@ -21,8 +23,11 @@ const defineMessageScreen = ( | |
bodyText, | ||
primaryAction, | ||
onPrimaryAction, | ||
secondarAction = tr("getHelp"), | ||
onSecondaryAction = () => open(sumoLink) | ||
secondaryAction = tr("getHelp"), | ||
onSecondaryAction = () => open(sumoLink), | ||
closeOnClick = true, | ||
totalPages = 0, | ||
currentPage = 0 | ||
) => { | ||
const body = | ||
typeof bodyText === "string" ? html`<p>${bodyText}</p>` : bodyText; | ||
|
@@ -34,10 +39,23 @@ const defineMessageScreen = ( | |
this.img = img; | ||
this.heading = heading; | ||
this.primaryAction = primaryAction; | ||
this.onPrimaryAction = onPrimaryAction; | ||
this.secondaryAction = secondarAction; | ||
this.onSecondaryAction = onSecondaryAction; | ||
this.secondaryAction = secondaryAction; | ||
if (closeOnClick) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to make this as stupid as it needs to be - WDYT: const closeAfter (f) => {
if(f){
f();
}
window.close();
} const defineMessageScreen = (
...
onSecondaryAction = () => closeAfter (()=>open(sumoLink)),
) Imho that is nicer to read then, as we dont need to recall what that one bool is flagging
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated this. Not my personal style, but feels more appropriate given this project's overall codebase. |
||
this.onPrimaryAction = function () { | ||
onPrimaryAction(); | ||
window.close(); | ||
}; | ||
this.onSecondaryAction = function () { | ||
onSecondaryAction(); | ||
window.close(); | ||
}; | ||
} else { | ||
this.onPrimaryAction = onPrimaryAction; | ||
this.onSecondaryAction = onSecondaryAction; | ||
} | ||
this.identifier = tag; | ||
this.totalPages = totalPages; | ||
this.currentPage = currentPage; | ||
render(body, this); | ||
} | ||
} | ||
|
@@ -105,6 +123,30 @@ defineMessageScreen( | |
null | ||
); | ||
|
||
// Need to start loop at 1 because of how the strings were added to l10n repo. | ||
for (let i = 1; i <= NUMBER_OF_ONBOARDING_PAGES; i++) { | ||
const isFinalScreen = i === NUMBER_OF_ONBOARDING_PAGES; | ||
defineMessageScreen( | ||
`onboarding-screen-${i}`, | ||
`onboarding-${i}.svg`, | ||
tr(`onboarding${i}_title`), | ||
html` <p>${tr(`onboarding${i}_body`)}</p> `, | ||
isFinalScreen ? tr("done") : tr("next"), | ||
() => { | ||
isFinalScreen | ||
? onboardingController.finishOnboarding() | ||
: onboardingController.nextOnboardingPage(); | ||
}, | ||
isFinalScreen ? tr(" ") : tr("skip"), // For final screen need a space - when using something like `null` there is a large vertical gap | ||
() => { | ||
isFinalScreen ? null : onboardingController.finishOnboarding(); | ||
}, | ||
false, | ||
NUMBER_OF_ONBOARDING_PAGES, | ||
i | ||
); | ||
} | ||
|
||
defineMessageScreen( | ||
"unsupported-os-message-screen", | ||
"message-os.svg", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -250,3 +250,28 @@ export const propertySum = (left, right, transform) => { | |
right.subscribe((r) => inner.set(transform(left.value, r))); | ||
return inner.readOnly; | ||
}; | ||
|
||
/** | ||
* Creates a "sum-type" property. | ||
* Takes 3 Properties {L,M,R} and a function (l,m,r)=>T | ||
* When L, M, or R changes calls the function | ||
* and updates the returned property. | ||
* | ||
* | ||
* @template T | ||
* @template L | ||
* @template M | ||
* @template R | ||
* @param {IBindable<L>} left - Left Hand Property | ||
* @param {IBindable<R>} right - Middle Hand Property | ||
* @param {IBindable<R>} right - Right Hand Property | ||
* @param {(arg0: L, arg1: M, arg2: R)=>T} transform - Called with the Property Value, must return the transformed value | ||
* @returns {ReadOnlyProperty<T>} - | ||
*/ | ||
export const propertySumTrio = (left, middle, right, transform) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A: First of really appreciate that you looked into that system :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I knew it was only a quick band aid fix... Will cherry pick that improved one after it is merged in. |
||
const inner = property(transform(left.value, middle.value, right.value)); | ||
left.subscribe((l) => inner.set(transform(l, middle.value, right.value))); | ||
middle.subscribe((m) => inner.set(transform(left.value, m, right.value))); | ||
right.subscribe((r) => inner.set(transform(left.value, middle.value, r))); | ||
return inner.readOnly; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this throw an oogly error if
storageRetrieval == undefined
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some more defensive code around here. Good call, thanks!