Skip to content
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

Merged
merged 10 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions src/assets/img/onboarding-1.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
79 changes: 79 additions & 0 deletions src/assets/img/onboarding-2.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
164 changes: 164 additions & 0 deletions src/assets/img/onboarding-3.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions src/background/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -26,6 +27,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(
Expand Down Expand Up @@ -61,6 +63,7 @@ class Main {
expose(this.extController);
expose(this.proxyHandler);
expose(this.conflictObserver);
expose(this.onboardingController);
expose(this.butterBarService);

this.#handlingEvent = false;
Expand Down
60 changes: 60 additions & 0 deletions src/background/onboarding.js
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);
}
19 changes: 12 additions & 7 deletions src/background/vpncontroller/vpncontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -317,28 +317,33 @@ const MOZILLA_VPN_SERVERS_KEY = "mozillaVpnServers";
* @param {T} defaultValue - The Default value, in case it does not exist.
* @returns {Promise<T>} - 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);
if (typeof storageRetrieval === "undefined") {
return defaultValue;
}
const returnValue = storageRetrieval[key];
Copy link
Member

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?

Copy link
Collaborator Author

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!


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
* @param {any} data - The state to replicate
* @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 });
Expand Down
51 changes: 48 additions & 3 deletions src/components/message-screen.js
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -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 {
Expand All @@ -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();
Expand All @@ -40,9 +44,18 @@ 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`
<vpn-titlebar title=${this.titleHeader}></vpn-titlebar>
<div class="inner">
Expand All @@ -51,6 +64,14 @@ export class MessageScreen extends LitElement {
<h1>${this.heading}</h1>
<slot></slot>
</div>
<div class="pagination">
${repeat(
paginationIndicators,
(item) => item.id,
(item) =>
html` <span class="holder"><span class="${item}"></span></span>`
)}
</div>
<div class="lower">
${when(
this.primaryAction,
Expand All @@ -59,7 +80,6 @@ export class MessageScreen extends LitElement {
class="primarybtn"
@click=${(e) => {
this.onPrimaryAction(this, e);
window.close();
}}
>
${this.primaryAction}
Expand All @@ -73,7 +93,6 @@ export class MessageScreen extends LitElement {
class="secondarybtn"
@click=${(e) => {
this.onSecondaryAction(this, e);
window.close();
}}
>
${this.secondaryAction}
Expand Down Expand Up @@ -135,6 +154,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;
Expand Down
52 changes: 47 additions & 5 deletions src/components/prefab-screens.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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;
Expand All @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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:
Remove the logic for closeOnClick as it only applys for the actions - and use a decorator for the ones where we need that functionality.

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

defineMessageScreen( ... false|true ... ) 


Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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);
}
}
Expand Down Expand Up @@ -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",
Expand Down
25 changes: 25 additions & 0 deletions src/shared/property.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A: First of really appreciate that you looked into that system :D
However i dont think that scales.
Consider cherry picking #144 - i think we can just rewrite property sum to take N amount of properties :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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;
};
3 changes: 3 additions & 0 deletions src/ui/browserAction/backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +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 butterBarService = await getExposedObject("ButterBarService");

export const ready = Promise.all([vpnController, proxyHandler]);
5 changes: 4 additions & 1 deletion src/ui/browserAction/popup.html
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@
<open-mozilla-vpn-message-screen
slot="MessageOpenMozillaVPN"
></open-mozilla-vpn-message-screen>
<!-- TODO: Split Tunnel Message, Onboarding, Update Message -->
<onboarding-screen-1 slot="onboarding-1"></onboarding-screen-1>
<onboarding-screen-2 slot="onboarding-2"></onboarding-screen-2>
<onboarding-screen-3 slot="onboarding-3"></onboarding-screen-3>
<!-- TODO: Split Tunnel Message, Update Message -->
<popup-browseraction slot="default"> </popup-browseraction>
</popup-condview>
</body>
Expand Down
Loading
Loading