From e8a40dad8d329cf8fceffe0fc9224cdc502ce171 Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Tue, 10 Dec 2024 13:53:20 -0800 Subject: [PATCH 1/8] FXVPN-10: onboarding --- .../extensionController.js | 32 ++++++++- src/background/vpncontroller/vpncontroller.js | 16 +++-- src/components/message-screen.js | 2 - src/components/prefab-screens.js | 69 +++++++++++++++++-- src/shared/property.js | 25 +++++++ src/ui/browserAction/popup.html | 11 ++- src/ui/browserAction/popupConditional.js | 31 +++++---- 7 files changed, 158 insertions(+), 28 deletions(-) diff --git a/src/background/extensionController/extensionController.js b/src/background/extensionController/extensionController.js index 26f6df69..955c60db 100644 --- a/src/background/extensionController/extensionController.js +++ b/src/background/extensionController/extensionController.js @@ -6,6 +6,7 @@ import { Component } from "../component.js"; import { VPNController, VPNState } from "../vpncontroller/index.js"; +import { fromStorage, putIntoStorage } from "../vpncontroller/vpncontroller.js"; import { property } from "../../shared/property.js"; import { PropertyType } from "../../shared/ipc.js"; import { @@ -17,6 +18,8 @@ import { isEquatable, } from "./states.js"; +const ONBOARDING_KEY = "mozillaVpnOnboarding"; + /** * * ExtensionController manages extension state and @@ -27,6 +30,9 @@ export class ExtensionController extends Component { static properties = { state: PropertyType.Bindable, toggleConnectivity: PropertyType.Function, + nextOnboardingPage: PropertyType.Function, + finishOnboarding: PropertyType.Function, + currentOnboardingPage: PropertyType.Bindable }; /** @@ -48,7 +54,13 @@ export class ExtensionController extends Component { /** @type {VPNState} */ clientState; - async init() {} + async init() { + this.#mCurrentOnboardingPage.value = await fromStorage( + browser.storage.local, + ONBOARDING_KEY, + 1 + ); + } toggleConnectivity() { if (this.#mState.value.enabled) { @@ -81,6 +93,23 @@ export class ExtensionController extends Component { return this.#mState.readOnly; } + get currentOnboardingPage() { + return this.#mCurrentOnboardingPage.readOnly; // true w/ or w/o readonly + } + + nextOnboardingPage() { + this.#mCurrentOnboardingPage.set(this.#mCurrentOnboardingPage.value + 1); + } + + finishOnboarding() { + this.#mCurrentOnboardingPage.set(4); + putIntoStorage( + 4, + browser.storage.local, + ONBOARDING_KEY + ); + } + /** * * @param {VPNState} newClientState @@ -137,5 +166,6 @@ export class ExtensionController extends Component { } #mState = property(new FirefoxVPNState()); + #mCurrentOnboardingPage = property(1); mKeepAliveConnection = false; } diff --git a/src/background/vpncontroller/vpncontroller.js b/src/background/vpncontroller/vpncontroller.js index 2e7a2eef..ff0b3f3b 100644 --- a/src/background/vpncontroller/vpncontroller.js +++ b/src/background/vpncontroller/vpncontroller.js @@ -269,17 +269,19 @@ const MOZILLA_VPN_SERVERS_KEY = "mozillaVpnServers"; * @param {T} defaultValue - The Default value, in case it does not exist. * @returns {Promise} - Returns a copy of the state, or the same in case of missing data. */ -async function fromStorage( +export async function fromStorage( storage = browser.storage.local, - key = MOZILLA_VPN_SERVERS_KEY, + key, defaultValue ) { - const { mozillaVpnServers } = await storage.get(key); - if (typeof mozillaVpnServers === "undefined") { + const storageRetrieval = await storage.get(key); + const returnValue = storageRetrieval[key]; + + if (typeof returnValue === "undefined") { return defaultValue; } // @ts-ignore - return mozillaVpnServers; + return returnValue; } /** data into storage, to make sure we can recreate it next time using @@ -287,10 +289,10 @@ async function fromStorage( * @param {browser.storage.StorageArea} storage - The storage area to look for * @param {String} key - The key to put the state in */ -function putIntoStorage( +export function putIntoStorage( data = {}, storage = browser.storage.local, - key = MOZILLA_VPN_SERVERS_KEY + key ) { // @ts-ignore storage.set({ [key]: data }); diff --git a/src/components/message-screen.js b/src/components/message-screen.js index 1a8c3780..7f520697 100644 --- a/src/components/message-screen.js +++ b/src/components/message-screen.js @@ -59,7 +59,6 @@ export class MessageScreen extends LitElement { class="primarybtn" @click=${(e) => { this.onPrimaryAction(this, e); - window.close(); }} > ${this.primaryAction} @@ -73,7 +72,6 @@ export class MessageScreen extends LitElement { class="secondarybtn" @click=${(e) => { this.onSecondaryAction(this, e); - window.close(); }} > ${this.secondaryAction} diff --git a/src/components/prefab-screens.js b/src/components/prefab-screens.js index b7457462..d7087871 100644 --- a/src/components/prefab-screens.js +++ b/src/components/prefab-screens.js @@ -5,6 +5,7 @@ import { html, render } from "../vendor/lit-all.min.js"; import { MessageScreen } from "./message-screen.js"; import { tr } from "../shared/i18n.js"; +import { extController } from "../ui/browserAction/backend.js"; const open = (url) => { browser.tabs.create({ @@ -21,8 +22,9 @@ const defineMessageScreen = ( bodyText, primaryAction, onPrimaryAction, - secondarAction = tr("getHelp"), - onSecondaryAction = () => open(sumoLink) + secondaryAction = tr("getHelp"), + onSecondaryAction = () => open(sumoLink), + closeOnClick = true ) => { const body = typeof bodyText === "string" ? html`

${bodyText}

` : bodyText; @@ -34,9 +36,20 @@ 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) { + this.onPrimaryAction = function() { + onPrimaryAction(); + window.close(); + } + this.onSecondaryAction = function() { + onSecondaryAction(); + window.close(); + } + } else { + this.onPrimaryAction = onPrimaryAction; + this.onSecondaryAction = onSecondaryAction; + } this.identifier = tag; render(body, this); } @@ -105,6 +118,52 @@ defineMessageScreen( null ); +defineMessageScreen( + "onboarding-screen-1", + "onboarding-1.svg", + tr("onboarding1_title"), + html`

${tr("onboarding1_body")}

`, + tr("next"), + () => { + extController.nextOnboardingPage(); + }, + tr("skip"), + () => { + extController.finishOnboarding(); + }, + false +); + +defineMessageScreen( + "onboarding-screen-2", + "onboarding-2.svg", + tr("onboarding2_title"), + html`

${tr("onboarding2_body")}

`, + tr("next"), + () => { + extController.nextOnboardingPage(); + }, + tr("skip"), + () => { + extController.finishOnboarding(); + }, + false +); + +defineMessageScreen( + "onboarding-screen-3", + "onboarding-3.svg", + tr("onboarding3_title"), + html`

${tr("onboarding3_body")}

`, + tr("done"), + () => { + extController.finishOnboarding(); + }, + tr(" "), // When using something like `null` there is a large vertical gap + null, + false +); + defineMessageScreen( "unsupported-os-message-screen", "message-os.svg", diff --git a/src/shared/property.js b/src/shared/property.js index d37a97b2..b8261762 100644 --- a/src/shared/property.js +++ b/src/shared/property.js @@ -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} left - Left Hand Property + * @param {IBindable} right - Middle Hand Property + * @param {IBindable} 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} - + */ +export const propertySumTrio = (left, middle, right, transform) => { + 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; +}; diff --git a/src/ui/browserAction/popup.html b/src/ui/browserAction/popup.html index a468b5aa..42a477b7 100644 --- a/src/ui/browserAction/popup.html +++ b/src/ui/browserAction/popup.html @@ -34,7 +34,16 @@ - + + + + diff --git a/src/ui/browserAction/popupConditional.js b/src/ui/browserAction/popupConditional.js index 7fadaa64..e50d7d05 100644 --- a/src/ui/browserAction/popupConditional.js +++ b/src/ui/browserAction/popupConditional.js @@ -3,9 +3,9 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ import { ConditionalView } from "../../components/conditional-view.js"; -import { propertySum } from "../../shared/property.js"; +import { propertySumTrio } from "../../shared/property.js"; import { Utils } from "../../shared/utils.js"; -import { vpnController } from "./backend.js"; +import { vpnController, extController } from "./backend.js"; export class PopUpConditionalView extends ConditionalView { constructor() { @@ -17,14 +17,16 @@ export class PopUpConditionalView extends ConditionalView { const deviceOs = await browser.runtime.getPlatformInfo(); const supportedPlatform = Utils.isSupportedOs(deviceOs.os); - propertySum( + propertySumTrio( vpnController.state, vpnController.featureList, - (state, features) => { + extController.currentOnboardingPage, + (state, features, currentPage) => { this.slotName = PopUpConditionalView.toSlotname( state, features, - supportedPlatform + supportedPlatform, + currentPage ); } ); @@ -48,9 +50,10 @@ export class PopUpConditionalView extends ConditionalView { * @param {State} state * @param {FeatureFlags} features * @param {Boolean} supportedPlatform + * @param {Number} currentOnboardingPage * @returns {String} */ - static toSlotname(state, features, supportedPlatform) { + static toSlotname(state, features, supportedPlatform, currentOnboardingPage) { if (!supportedPlatform && !features.webExtension) { return "MessageOSNotSupported"; } @@ -72,12 +75,16 @@ export class PopUpConditionalView extends ConditionalView { if (!state.subscribed) { return "MessageSubscription"; } - /** - * TODO: - * if( did not have onboarding){ - * return "onBoarding" - * } - */ + if (currentOnboardingPage == 1) { + return "onboarding-1" + } + if (currentOnboardingPage == 2) { + return "onboarding-2" + } + if (currentOnboardingPage == 3) { + return "onboarding-3" + } + return "default"; } } From 677e2335e18a77851b081dda3dd5f46df4e6e7f6 Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 13 Dec 2024 10:02:18 -0800 Subject: [PATCH 2/8] PR feedback --- src/assets/img/onboarding-1.svg | 55 ++++++ src/assets/img/onboarding-2.svg | 79 +++++++++ src/assets/img/onboarding-3.svg | 164 ++++++++++++++++++ .../extensionController.js | 31 +--- src/background/main.js | 3 + src/background/onboarding.js | 66 +++++++ src/background/vpncontroller/vpncontroller.js | 3 + src/components/prefab-screens.js | 12 +- src/ui/browserAction/backend.js | 1 + src/ui/browserAction/popupConditional.js | 4 +- 10 files changed, 380 insertions(+), 38 deletions(-) create mode 100644 src/assets/img/onboarding-1.svg create mode 100644 src/assets/img/onboarding-2.svg create mode 100644 src/assets/img/onboarding-3.svg create mode 100644 src/background/onboarding.js diff --git a/src/assets/img/onboarding-1.svg b/src/assets/img/onboarding-1.svg new file mode 100644 index 00000000..1387beb5 --- /dev/null +++ b/src/assets/img/onboarding-1.svg @@ -0,0 +1,55 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/assets/img/onboarding-2.svg b/src/assets/img/onboarding-2.svg new file mode 100644 index 00000000..51eaf0fe --- /dev/null +++ b/src/assets/img/onboarding-2.svg @@ -0,0 +1,79 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/assets/img/onboarding-3.svg b/src/assets/img/onboarding-3.svg new file mode 100644 index 00000000..1356d279 --- /dev/null +++ b/src/assets/img/onboarding-3.svg @@ -0,0 +1,164 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/background/extensionController/extensionController.js b/src/background/extensionController/extensionController.js index 955c60db..8cb2126d 100644 --- a/src/background/extensionController/extensionController.js +++ b/src/background/extensionController/extensionController.js @@ -18,8 +18,6 @@ import { isEquatable, } from "./states.js"; -const ONBOARDING_KEY = "mozillaVpnOnboarding"; - /** * * ExtensionController manages extension state and @@ -30,9 +28,6 @@ export class ExtensionController extends Component { static properties = { state: PropertyType.Bindable, toggleConnectivity: PropertyType.Function, - nextOnboardingPage: PropertyType.Function, - finishOnboarding: PropertyType.Function, - currentOnboardingPage: PropertyType.Bindable }; /** @@ -54,13 +49,7 @@ export class ExtensionController extends Component { /** @type {VPNState} */ clientState; - async init() { - this.#mCurrentOnboardingPage.value = await fromStorage( - browser.storage.local, - ONBOARDING_KEY, - 1 - ); - } + async init() { } toggleConnectivity() { if (this.#mState.value.enabled) { @@ -93,23 +82,6 @@ export class ExtensionController extends Component { return this.#mState.readOnly; } - get currentOnboardingPage() { - return this.#mCurrentOnboardingPage.readOnly; // true w/ or w/o readonly - } - - nextOnboardingPage() { - this.#mCurrentOnboardingPage.set(this.#mCurrentOnboardingPage.value + 1); - } - - finishOnboarding() { - this.#mCurrentOnboardingPage.set(4); - putIntoStorage( - 4, - browser.storage.local, - ONBOARDING_KEY - ); - } - /** * * @param {VPNState} newClientState @@ -166,6 +138,5 @@ export class ExtensionController extends Component { } #mState = property(new FirefoxVPNState()); - #mCurrentOnboardingPage = property(1); mKeepAliveConnection = false; } diff --git a/src/background/main.js b/src/background/main.js index 97a6c1e6..9fb36f27 100644 --- a/src/background/main.js +++ b/src/background/main.js @@ -10,6 +10,7 @@ import { ToolbarIconHandler } from "./toolbarIconHandler.js"; import { VPNController } from "./vpncontroller/index.js"; import { ExtensionController } from "./extensionController/index.js"; +import { OnboardingController } from "./onboarding.js"; import { expose } from "../shared/ipc.js"; import { TabReloader } from "./tabReloader.js"; @@ -24,6 +25,7 @@ class Main { conflictObserver = new ConflictObserver(); vpnController = new VPNController(this); extController = new ExtensionController(this, this.vpnController); + onboardingController = new OnboardingController(this); logger = new Logger(this); proxyHandler = new ProxyHandler(this, this.vpnController); requestHandler = new RequestHandler( @@ -54,6 +56,7 @@ class Main { expose(this.extController); expose(this.proxyHandler); expose(this.conflictObserver); + expose(this.onboardingController); this.#handlingEvent = false; this.#processPendingEvents(); diff --git a/src/background/onboarding.js b/src/background/onboarding.js new file mode 100644 index 00000000..7765cfd6 --- /dev/null +++ b/src/background/onboarding.js @@ -0,0 +1,66 @@ +/* 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; +const FIRST_UNUSED_PAGE = 4; + +// const log = Logger.logger("RequestHandler"); +let self; + +/** + * 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(4); + putIntoStorage( + FIRST_UNUSED_PAGE, + browser.storage.local, + ONBOARDING_KEY + ); + } + + #mCurrentOnboardingPage = property(FIRST_PAGE); +} diff --git a/src/background/vpncontroller/vpncontroller.js b/src/background/vpncontroller/vpncontroller.js index ff0b3f3b..3b99aacf 100644 --- a/src/background/vpncontroller/vpncontroller.js +++ b/src/background/vpncontroller/vpncontroller.js @@ -275,6 +275,9 @@ export async function fromStorage( defaultValue ) { const storageRetrieval = await storage.get(key); + if (typeof storageRetrieval === "undefined") { + return defaultValue; + } const returnValue = storageRetrieval[key]; if (typeof returnValue === "undefined") { diff --git a/src/components/prefab-screens.js b/src/components/prefab-screens.js index d7087871..98aea8d0 100644 --- a/src/components/prefab-screens.js +++ b/src/components/prefab-screens.js @@ -5,7 +5,7 @@ import { html, render } from "../vendor/lit-all.min.js"; import { MessageScreen } from "./message-screen.js"; import { tr } from "../shared/i18n.js"; -import { extController } from "../ui/browserAction/backend.js"; +import { onboardingController } from "../ui/browserAction/backend.js"; const open = (url) => { browser.tabs.create({ @@ -125,11 +125,11 @@ defineMessageScreen( html`

${tr("onboarding1_body")}

`, tr("next"), () => { - extController.nextOnboardingPage(); + onboardingController.nextOnboardingPage(); }, tr("skip"), () => { - extController.finishOnboarding(); + onboardingController.finishOnboarding(); }, false ); @@ -141,11 +141,11 @@ defineMessageScreen( html`

${tr("onboarding2_body")}

`, tr("next"), () => { - extController.nextOnboardingPage(); + onboardingController.nextOnboardingPage(); }, tr("skip"), () => { - extController.finishOnboarding(); + onboardingController.finishOnboarding(); }, false ); @@ -157,7 +157,7 @@ defineMessageScreen( html`

${tr("onboarding3_body")}

`, tr("done"), () => { - extController.finishOnboarding(); + onboardingController.finishOnboarding(); }, tr(" "), // When using something like `null` there is a large vertical gap null, diff --git a/src/ui/browserAction/backend.js b/src/ui/browserAction/backend.js index 9cb0a6dc..219477a4 100644 --- a/src/ui/browserAction/backend.js +++ b/src/ui/browserAction/backend.js @@ -27,5 +27,6 @@ import { getExposedObject } from "../../shared/ipc.js"; export const vpnController = await getExposedObject("VPNController"); export const extController = await getExposedObject("ExtensionController"); export const proxyHandler = await getExposedObject("ProxyHandler"); +export const onboardingController = await getExposedObject("OnboardingController"); export const ready = Promise.all([vpnController, proxyHandler]); diff --git a/src/ui/browserAction/popupConditional.js b/src/ui/browserAction/popupConditional.js index e50d7d05..ae9a0b0d 100644 --- a/src/ui/browserAction/popupConditional.js +++ b/src/ui/browserAction/popupConditional.js @@ -5,7 +5,7 @@ import { ConditionalView } from "../../components/conditional-view.js"; import { propertySumTrio } from "../../shared/property.js"; import { Utils } from "../../shared/utils.js"; -import { vpnController, extController } from "./backend.js"; +import { vpnController, onboardingController } from "./backend.js"; export class PopUpConditionalView extends ConditionalView { constructor() { @@ -20,7 +20,7 @@ export class PopUpConditionalView extends ConditionalView { propertySumTrio( vpnController.state, vpnController.featureList, - extController.currentOnboardingPage, + onboardingController.currentOnboardingPage, (state, features, currentPage) => { this.slotName = PopUpConditionalView.toSlotname( state, From afa3aaa756ac0ad3472e7953b49c8b43e5d56dd7 Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 13 Dec 2024 12:28:00 -0800 Subject: [PATCH 3/8] better reuse --- src/background/onboarding.js | 5 +- src/components/message-screen.js | 44 +++++++++++++- src/components/prefab-screens.js | 73 +++++++++--------------- src/ui/browserAction/popupConditional.js | 12 +--- 4 files changed, 76 insertions(+), 58 deletions(-) diff --git a/src/background/onboarding.js b/src/background/onboarding.js index 7765cfd6..b62b497e 100644 --- a/src/background/onboarding.js +++ b/src/background/onboarding.js @@ -12,7 +12,8 @@ import { fromStorage, putIntoStorage } from "./vpncontroller/vpncontroller.js"; const ONBOARDING_KEY = "mozillaVpnOnboarding"; const FIRST_PAGE = 1; -const FIRST_UNUSED_PAGE = 4; +export const NUMBER_OF_ONBOARDING_PAGES = 3; +const FIRST_UNUSED_PAGE = NUMBER_OF_ONBOARDING_PAGES + 1; // const log = Logger.logger("RequestHandler"); let self; @@ -54,7 +55,7 @@ export class OnboardingController extends Component { } finishOnboarding() { - this.#mCurrentOnboardingPage.set(4); + this.#mCurrentOnboardingPage.set(FIRST_UNUSED_PAGE); putIntoStorage( FIRST_UNUSED_PAGE, browser.storage.local, diff --git a/src/components/message-screen.js b/src/components/message-screen.js index 7f520697..f57c3479 100644 --- a/src/components/message-screen.js +++ b/src/components/message-screen.js @@ -2,7 +2,7 @@ * 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/. */ -import { html, LitElement, when, css } from "../vendor/lit-all.min.js"; +import { html, LitElement, when, repeat, css } from "../vendor/lit-all.min.js"; import { fontStyling } from "./styles.js"; import "./titlebar.js"; @@ -17,6 +17,8 @@ import "./titlebar.js"; * - onPrimaryAction -> A function to call when the primary button is clicked * - secondaryAction -> The 2ndary button text * - onSecondaryAction -> A function to call when the 2ndary action is clicked. + * - totalPages -> The number of pages to show in pagination + * - currentPage -> The active page for pagination */ export class MessageScreen extends LitElement { @@ -29,6 +31,8 @@ export class MessageScreen extends LitElement { secondaryAction: { type: String }, onSecondaryAction: { type: Function }, identifier: { type: String }, + totalPages: { type: Number }, + currentPage: { type: Number }, }; constructor() { super(); @@ -40,9 +44,16 @@ export class MessageScreen extends LitElement { this.onPrimaryAction = () => {}; this.onSecondaryAction = () => {}; this.identifier = ""; + this.totalPages = 0; + this.currentPage = 0; } render() { + let paginationIndicators = []; + for (let i = 0; i < this.totalPages; i++) { + paginationIndicators.push(((i+1) === this.currentPage) ? "circle active" : "circle"); + } + return html`
@@ -51,6 +62,11 @@ export class MessageScreen extends LitElement {

${this.heading}

+
${when( this.primaryAction, @@ -133,6 +149,32 @@ export class MessageScreen extends LitElement { inline-size: 111px; } + .pagination { + box-sizing: border-box; + position: relative; + width: 100%; + margin: 0px 0px 25px; + justify-content: center; + right: 4px; // This must be half the width of .circle to truly center it. + } + + .holder { + display: inline-block; + width: 14px; + } + + .circle { + position: absolute; + width: 8px; + height: 8px; + background: var(--grey30); + border-radius: 100%; + } + + .active { + background: var(--action-button-color); + } + h1 { font-family: var(--font-family-bold); margin-block: 16px; diff --git a/src/components/prefab-screens.js b/src/components/prefab-screens.js index 98aea8d0..53a310b0 100644 --- a/src/components/prefab-screens.js +++ b/src/components/prefab-screens.js @@ -24,7 +24,9 @@ const defineMessageScreen = ( onPrimaryAction, secondaryAction = tr("getHelp"), onSecondaryAction = () => open(sumoLink), - closeOnClick = true + closeOnClick = true, + totalPages = 0, + currentPage = 0 ) => { const body = typeof bodyText === "string" ? html`

${bodyText}

` : bodyText; @@ -51,6 +53,8 @@ const defineMessageScreen = ( this.onSecondaryAction = onSecondaryAction; } this.identifier = tag; + this.totalPages = totalPages; + this.currentPage = currentPage; render(body, this); } } @@ -118,51 +122,28 @@ defineMessageScreen( null ); -defineMessageScreen( - "onboarding-screen-1", - "onboarding-1.svg", - tr("onboarding1_title"), - html`

${tr("onboarding1_body")}

`, - tr("next"), - () => { - onboardingController.nextOnboardingPage(); - }, - tr("skip"), - () => { - onboardingController.finishOnboarding(); - }, - false -); - -defineMessageScreen( - "onboarding-screen-2", - "onboarding-2.svg", - tr("onboarding2_title"), - html`

${tr("onboarding2_body")}

`, - tr("next"), - () => { - onboardingController.nextOnboardingPage(); - }, - tr("skip"), - () => { - onboardingController.finishOnboarding(); - }, - false -); - -defineMessageScreen( - "onboarding-screen-3", - "onboarding-3.svg", - tr("onboarding3_title"), - html`

${tr("onboarding3_body")}

`, - tr("done"), - () => { - onboardingController.finishOnboarding(); - }, - tr(" "), // When using something like `null` there is a large vertical gap - null, - false -); +const NUMBER_ONBOARDING_SCREENS = 3; +// Need to start loop at 1 because of how the strings were added to l10n repo. +for (let i = 1; i <= NUMBER_ONBOARDING_SCREENS; i++) { + const isFinalScreen = (i === NUMBER_ONBOARDING_SCREENS); + defineMessageScreen( + `onboarding-screen-${i}`, + `onboarding-${i}.svg`, + tr(`onboarding${i}_title`), + html`

${tr(`onboarding${i}_body`)}

`, + 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_ONBOARDING_SCREENS, + i + ); +} defineMessageScreen( "unsupported-os-message-screen", diff --git a/src/ui/browserAction/popupConditional.js b/src/ui/browserAction/popupConditional.js index ae9a0b0d..e7639076 100644 --- a/src/ui/browserAction/popupConditional.js +++ b/src/ui/browserAction/popupConditional.js @@ -5,7 +5,7 @@ import { ConditionalView } from "../../components/conditional-view.js"; import { propertySumTrio } from "../../shared/property.js"; import { Utils } from "../../shared/utils.js"; -import { vpnController, onboardingController } from "./backend.js"; +import { vpnController, onboardingController, NUMBER_OF_ONBOARDING_PAGES } from "./backend.js"; export class PopUpConditionalView extends ConditionalView { constructor() { @@ -75,14 +75,8 @@ export class PopUpConditionalView extends ConditionalView { if (!state.subscribed) { return "MessageSubscription"; } - if (currentOnboardingPage == 1) { - return "onboarding-1" - } - if (currentOnboardingPage == 2) { - return "onboarding-2" - } - if (currentOnboardingPage == 3) { - return "onboarding-3" + if (currentOnboardingPage >= 1 && currentOnboardingPage <= NUMBER_OF_ONBOARDING_PAGES) { + return `onboarding-${currentOnboardingPage}` } return "default"; From 20a6ef7d0c5f02b651a7140a99ceb4446386c983 Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 13 Dec 2024 12:28:46 -0800 Subject: [PATCH 4/8] remove unused artifacts --- src/background/extensionController/extensionController.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/background/extensionController/extensionController.js b/src/background/extensionController/extensionController.js index 8cb2126d..26f6df69 100644 --- a/src/background/extensionController/extensionController.js +++ b/src/background/extensionController/extensionController.js @@ -6,7 +6,6 @@ import { Component } from "../component.js"; import { VPNController, VPNState } from "../vpncontroller/index.js"; -import { fromStorage, putIntoStorage } from "../vpncontroller/vpncontroller.js"; import { property } from "../../shared/property.js"; import { PropertyType } from "../../shared/ipc.js"; import { @@ -49,7 +48,7 @@ export class ExtensionController extends Component { /** @type {VPNState} */ clientState; - async init() { } + async init() {} toggleConnectivity() { if (this.#mState.value.enabled) { From a191fbda9c1bc7399d1df73111bc202204e77f22 Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 13 Dec 2024 12:36:10 -0800 Subject: [PATCH 5/8] reuse the constant better --- src/components/prefab-screens.js | 8 ++++---- src/ui/browserAction/popupConditional.js | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/components/prefab-screens.js b/src/components/prefab-screens.js index 53a310b0..a11e9159 100644 --- a/src/components/prefab-screens.js +++ b/src/components/prefab-screens.js @@ -6,6 +6,7 @@ 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({ @@ -122,10 +123,9 @@ defineMessageScreen( null ); -const NUMBER_ONBOARDING_SCREENS = 3; // Need to start loop at 1 because of how the strings were added to l10n repo. -for (let i = 1; i <= NUMBER_ONBOARDING_SCREENS; i++) { - const isFinalScreen = (i === NUMBER_ONBOARDING_SCREENS); +for (let i = 1; i <= NUMBER_OF_ONBOARDING_PAGES; i++) { + const isFinalScreen = (i === NUMBER_OF_ONBOARDING_PAGES); defineMessageScreen( `onboarding-screen-${i}`, `onboarding-${i}.svg`, @@ -140,7 +140,7 @@ for (let i = 1; i <= NUMBER_ONBOARDING_SCREENS; i++) { isFinalScreen ? null : onboardingController.finishOnboarding(); }, false, - NUMBER_ONBOARDING_SCREENS, + NUMBER_OF_ONBOARDING_PAGES, i ); } diff --git a/src/ui/browserAction/popupConditional.js b/src/ui/browserAction/popupConditional.js index e7639076..b8507a26 100644 --- a/src/ui/browserAction/popupConditional.js +++ b/src/ui/browserAction/popupConditional.js @@ -5,7 +5,8 @@ import { ConditionalView } from "../../components/conditional-view.js"; import { propertySumTrio } from "../../shared/property.js"; import { Utils } from "../../shared/utils.js"; -import { vpnController, onboardingController, NUMBER_OF_ONBOARDING_PAGES } from "./backend.js"; +import { vpnController, onboardingController } from "./backend.js"; +import { NUMBER_OF_ONBOARDING_PAGES } from "../../background/onboarding.js" export class PopUpConditionalView extends ConditionalView { constructor() { From 6e69e751c44604dac6324386469e49ea0141f7fd Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 13 Dec 2024 14:15:31 -0800 Subject: [PATCH 6/8] remove unused stuff --- src/background/onboarding.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/background/onboarding.js b/src/background/onboarding.js index b62b497e..68ddacad 100644 --- a/src/background/onboarding.js +++ b/src/background/onboarding.js @@ -15,9 +15,6 @@ const FIRST_PAGE = 1; export const NUMBER_OF_ONBOARDING_PAGES = 3; const FIRST_UNUSED_PAGE = NUMBER_OF_ONBOARDING_PAGES + 1; -// const log = Logger.logger("RequestHandler"); -let self; - /** * Handles onboarding. * From 5b2f4225cd0bf5287b7598dfb2c5495ea106f0db Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 13 Dec 2024 14:33:54 -0800 Subject: [PATCH 7/8] happy linter, happy devs --- src/background/onboarding.js | 22 +++++++++------------- src/components/message-screen.js | 11 ++++++++--- src/components/prefab-screens.js | 16 +++++++++------- src/ui/browserAction/backend.js | 4 +++- src/ui/browserAction/popup.html | 12 +++--------- src/ui/browserAction/popupConditional.js | 9 ++++++--- 6 files changed, 38 insertions(+), 36 deletions(-) diff --git a/src/background/onboarding.js b/src/background/onboarding.js index 68ddacad..8fa0a96a 100644 --- a/src/background/onboarding.js +++ b/src/background/onboarding.js @@ -21,9 +21,9 @@ const FIRST_UNUSED_PAGE = NUMBER_OF_ONBOARDING_PAGES + 1; */ export class OnboardingController extends Component { static properties = { - nextOnboardingPage: PropertyType.Function, - finishOnboarding: PropertyType.Function, - currentOnboardingPage: PropertyType.Bindable + nextOnboardingPage: PropertyType.Function, + finishOnboarding: PropertyType.Function, + currentOnboardingPage: PropertyType.Bindable, }; /** @@ -37,14 +37,14 @@ export class OnboardingController extends Component { async init() { this.#mCurrentOnboardingPage.value = await fromStorage( - browser.storage.local, - ONBOARDING_KEY, - FIRST_PAGE + browser.storage.local, + ONBOARDING_KEY, + FIRST_PAGE ); - } + } get currentOnboardingPage() { - return this.#mCurrentOnboardingPage.readOnly; + return this.#mCurrentOnboardingPage.readOnly; } nextOnboardingPage() { @@ -53,11 +53,7 @@ export class OnboardingController extends Component { finishOnboarding() { this.#mCurrentOnboardingPage.set(FIRST_UNUSED_PAGE); - putIntoStorage( - FIRST_UNUSED_PAGE, - browser.storage.local, - ONBOARDING_KEY - ); + putIntoStorage(FIRST_UNUSED_PAGE, browser.storage.local, ONBOARDING_KEY); } #mCurrentOnboardingPage = property(FIRST_PAGE); diff --git a/src/components/message-screen.js b/src/components/message-screen.js index f57c3479..9ded05d6 100644 --- a/src/components/message-screen.js +++ b/src/components/message-screen.js @@ -51,7 +51,9 @@ export class MessageScreen extends LitElement { render() { let paginationIndicators = []; for (let i = 0; i < this.totalPages; i++) { - paginationIndicators.push(((i+1) === this.currentPage) ? "circle active" : "circle"); + paginationIndicators.push( + i + 1 === this.currentPage ? "circle active" : "circle" + ); } return html` @@ -63,8 +65,11 @@ export class MessageScreen extends LitElement {
diff --git a/src/components/prefab-screens.js b/src/components/prefab-screens.js index a11e9159..256fdecc 100644 --- a/src/components/prefab-screens.js +++ b/src/components/prefab-screens.js @@ -6,7 +6,7 @@ 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" +import { NUMBER_OF_ONBOARDING_PAGES } from "../background/onboarding.js"; const open = (url) => { browser.tabs.create({ @@ -41,14 +41,14 @@ const defineMessageScreen = ( this.primaryAction = primaryAction; this.secondaryAction = secondaryAction; if (closeOnClick) { - this.onPrimaryAction = function() { + this.onPrimaryAction = function () { onPrimaryAction(); window.close(); - } - this.onSecondaryAction = function() { + }; + this.onSecondaryAction = function () { onSecondaryAction(); window.close(); - } + }; } else { this.onPrimaryAction = onPrimaryAction; this.onSecondaryAction = onSecondaryAction; @@ -125,7 +125,7 @@ defineMessageScreen( // 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); + const isFinalScreen = i === NUMBER_OF_ONBOARDING_PAGES; defineMessageScreen( `onboarding-screen-${i}`, `onboarding-${i}.svg`, @@ -133,7 +133,9 @@ for (let i = 1; i <= NUMBER_OF_ONBOARDING_PAGES; i++) { html`

${tr(`onboarding${i}_body`)}

`, isFinalScreen ? tr("done") : tr("next"), () => { - isFinalScreen ? onboardingController.finishOnboarding() : onboardingController.nextOnboardingPage(); + 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 () => { diff --git a/src/ui/browserAction/backend.js b/src/ui/browserAction/backend.js index da1a0b51..e0ddbe64 100644 --- a/src/ui/browserAction/backend.js +++ b/src/ui/browserAction/backend.js @@ -27,7 +27,9 @@ import { getExposedObject } from "../../shared/ipc.js"; export const vpnController = await getExposedObject("VPNController"); export const extController = await getExposedObject("ExtensionController"); export const proxyHandler = await getExposedObject("ProxyHandler"); -export const onboardingController = await getExposedObject("OnboardingController"); +export const onboardingController = await getExposedObject( + "OnboardingController" +); export const butterBarService = await getExposedObject("ButterBarService"); export const ready = Promise.all([vpnController, proxyHandler]); diff --git a/src/ui/browserAction/popup.html b/src/ui/browserAction/popup.html index 61845653..0cebbaec 100644 --- a/src/ui/browserAction/popup.html +++ b/src/ui/browserAction/popup.html @@ -35,15 +35,9 @@ - - - + + + diff --git a/src/ui/browserAction/popupConditional.js b/src/ui/browserAction/popupConditional.js index b8507a26..343ebbd5 100644 --- a/src/ui/browserAction/popupConditional.js +++ b/src/ui/browserAction/popupConditional.js @@ -6,7 +6,7 @@ import { ConditionalView } from "../../components/conditional-view.js"; import { propertySumTrio } from "../../shared/property.js"; import { Utils } from "../../shared/utils.js"; import { vpnController, onboardingController } from "./backend.js"; -import { NUMBER_OF_ONBOARDING_PAGES } from "../../background/onboarding.js" +import { NUMBER_OF_ONBOARDING_PAGES } from "../../background/onboarding.js"; export class PopUpConditionalView extends ConditionalView { constructor() { @@ -76,8 +76,11 @@ export class PopUpConditionalView extends ConditionalView { if (!state.subscribed) { return "MessageSubscription"; } - if (currentOnboardingPage >= 1 && currentOnboardingPage <= NUMBER_OF_ONBOARDING_PAGES) { - return `onboarding-${currentOnboardingPage}` + if ( + currentOnboardingPage >= 1 && + currentOnboardingPage <= NUMBER_OF_ONBOARDING_PAGES + ) { + return `onboarding-${currentOnboardingPage}`; } return "default"; From 1e90c4b64438b4e11a766efca9a9bda3f03a5f1f Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Mon, 16 Dec 2024 12:00:00 -0800 Subject: [PATCH 8/8] PR feedback --- src/components/prefab-screens.js | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/components/prefab-screens.js b/src/components/prefab-screens.js index 30b439d8..10866617 100644 --- a/src/components/prefab-screens.js +++ b/src/components/prefab-screens.js @@ -16,6 +16,13 @@ const open = (url) => { const sumoLink = "https://support.mozilla.org/products/firefox-private-network-vpn"; +const closeAfter = (f) => { + if(f){ + f(); + } + window.close(); +} + const defineMessageScreen = ( tag, img, @@ -24,8 +31,7 @@ const defineMessageScreen = ( primaryAction, onPrimaryAction, secondaryAction = tr("getHelp"), - onSecondaryAction = () => open(sumoLink), - closeOnClick = true, + onSecondaryAction = () => closeAfter (()=>open(sumoLink)), totalPages = 0, currentPage = 0 ) => { @@ -40,19 +46,8 @@ const defineMessageScreen = ( this.heading = heading; this.primaryAction = primaryAction; this.secondaryAction = secondaryAction; - if (closeOnClick) { - this.onPrimaryAction = function () { - onPrimaryAction(); - window.close(); - }; - this.onSecondaryAction = function () { - onSecondaryAction(); - window.close(); - }; - } else { - this.onPrimaryAction = onPrimaryAction; - this.onSecondaryAction = onSecondaryAction; - } + this.onPrimaryAction = onPrimaryAction; + this.onSecondaryAction = onSecondaryAction; this.identifier = tag; this.totalPages = totalPages; this.currentPage = currentPage; @@ -78,7 +73,7 @@ defineMessageScreen( tr("bodySubscribeNow"), tr("btnSubscribeNow"), () => { - open("https://www.mozilla.org/products/vpn#pricing"); + () => closeAfter (()=>open("https://www.mozilla.org/products/vpn#pricing")); } ); @@ -89,7 +84,7 @@ defineMessageScreen( tr("bodyNeedsUpdate2"), tr("btnDownloadNow"), () => { - open("https://www.mozilla.org/products/vpn/download/"); + () => closeAfter (()=>open("https://www.mozilla.org/products/vpn/download/")); } ); @@ -110,7 +105,7 @@ defineMessageScreen( `, tr("btnDownloadNow"), () => { - open("https://www.mozilla.org/products/vpn/download/"); + () => closeAfter (()=>open("https://www.mozilla.org/products/vpn/download/")); } ); @@ -141,7 +136,6 @@ for (let i = 1; i <= NUMBER_OF_ONBOARDING_PAGES; i++) { () => { isFinalScreen ? null : onboardingController.finishOnboarding(); }, - false, NUMBER_OF_ONBOARDING_PAGES, i );