FXVPN-10: onboarding flow #142

mcleinman · 2024-12-10T22:01:46Z

These should match onboarding mocks from figma.

Associated PR is up for the string: mozilla-l10n/mozilla-vpn-extension-l10n#3

Updated video: https://github.com/user-attachments/assets/317e48e7-9982-4489-ad19-8651dd5f21a6

The scroll bars in the video on page 2 are due to my browser settings, and won't be seen by most people.

strseb · 2024-12-11T16:21:31Z

src/shared/property.js

+ * @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) => {


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 :)

Thanks! I knew it was only a quick band aid fix... Will cherry pick that improved one after it is merged in.

strseb · 2024-12-11T16:26:57Z

src/background/extensionController/extensionController.js

Nitpick:

* ExtensionController manages extension state and * provides a method to the popup for disabling and enabling * the "Firefox VPN".

I don't think we should therfore have onboarding here.
All of the code here looks great, but i'd prefer it moved into onboarding.js and we expose that in main.js similar to the others :)

Good call, done

strseb · 2024-12-11T16:29:00Z

src/background/extensionController/extensionController.js

+  finishOnboarding() {
+    this.#mCurrentOnboardingPage.set(4);
+    putIntoStorage(
+      4,


Consider using constants to denote magic numbers Unless you have seen the figma it's not clear the max onboarding page is "4"

Also what happens if we add a new onboarding page? Users would then be seeing that one no?
(Not sure if this is wanted or not)

Will update away from magic numbers, thanks.

We do want users to have the possibility of later seeing new onboarding pages - Lesley mentioned this was desired (and sometimes difficult) w/ MAC.

strseb · 2024-12-11T16:34:45Z

src/ui/browserAction/popup.html

@@ -34,7 +34,16 @@
      <open-mozilla-vpn-message-screen
        slot="MessageOpenMozillaVPN"
      ></open-mozilla-vpn-message-screen>
-      <!-- TODO: Split Tunnel Message, Onboarding, Update Message -->
+      <onboarding-screen-1


With this approach i can see why the pagination would be hard to add (agreed)

WDYT:
one new Screen Element -> "Onboarding" that is shown here if onboarding is not complete.

The Onboarding screen then would be something like

<main> <conditional-view slotName=${this.onboardingCount} > <slot=1 onboarding-screen-1> <slot=2 onboarding-screen-2> <slot=3 onboarding-screen-3> <slot=4 onboarding-screen-4> </conditional-view> <pagination> <button-next> </main>

(this is also a nonblocking suggestion, if we REALLY dont want the indicators let's not add this )

I'm not fixated on the pagination either (MAC doesn't have them) but I do think they're a very nice touch, if not too painful.

Pagination is now in here - will update the video once I add in the new fourth screen.

mcleinman · 2024-12-12T20:40:13Z

We got a new screen added into the mocs yesterday, so putting this on draft until I can update the PR.

lesleyjanenorton · 2024-12-12T20:48:16Z

We got a new screen added into the mocs yesterday, so putting this on draft until I can update the PR.

@mcleinman do you mean the update the settings panel? if not, mind shooting me a link to the new screen? thanks!

lesleyjanenorton · 2024-12-12T20:55:53Z

src/background/vpncontroller/vpncontroller.js

-  const { mozillaVpnServers } = await storage.get(key);
-  if (typeof mozillaVpnServers === "undefined") {
+  const storageRetrieval = await storage.get(key);
+  const returnValue = storageRetrieval[key];


Will this throw an oogly error if storageRetrieval == undefined?

I added some more defensive code around here. Good call, thanks!

mcleinman · 2024-12-12T23:36:08Z

We got a new screen added into the mocs yesterday, so putting this on draft until I can update the PR.

@mcleinman do you mean the update the settings panel? if not, mind shooting me a link to the new screen? thanks!

The settings panel also came with a new onboarding panel: https://www.figma.com/design/s13B7zs27cadZXUyvxobgW/Mozilla-VPN-Extension?node-id=1822-31036&node-type=section&t=zKYBItOJwJmrfOoF-0

mcleinman · 2024-12-13T22:05:57Z

I believe this is ready for review.

The mocks got updated in the middle of this week with a new fourth onboarding screen. I'm inclined to add that after this is merged - I've started working on it, but there are open questions around copy and images, and I'm not sure this needs to be held up on it. Also open to putting this one back on draft if we think it's better to do it all together.

lesleyjanenorton · 2024-12-13T22:07:48Z

src/background/onboarding.js

+const FIRST_UNUSED_PAGE = NUMBER_OF_ONBOARDING_PAGES + 1;
+
+// const log = Logger.logger("RequestHandler");
+let self;


unused I think?

Removed, whoops. Thanks.

lesleyjanenorton · 2024-12-13T22:08:38Z

src/background/onboarding.js

+export const NUMBER_OF_ONBOARDING_PAGES = 3;
+const FIRST_UNUSED_PAGE = NUMBER_OF_ONBOARDING_PAGES + 1;
+
+// const log = Logger.logger("RequestHandler");


Let's remove this or comment it in and use

Removed, whoops. Thanks.

strseb

r+wc, thank you !!

strseb · 2024-12-16T16:17:57Z

src/components/prefab-screens.js

-      this.secondaryAction = secondarAction;
-      this.onSecondaryAction = onSecondaryAction;
+      this.secondaryAction = secondaryAction;
+      if (closeOnClick) {


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 ... )

I've updated this. Not my personal style, but feels more appropriate given this project's overall codebase.

- `computed` can be created without a type, so let's remove the class backing it. In #142 Matt hit a thing where a sum of more then 2 elements is required, let's refactor propertySum to work with N elements, and add tests for it :)

FXVPN-10: onboarding

e8a40da

mcleinman requested review from lesleyjanenorton and strseb as code owners December 10, 2024 22:01

strseb requested changes Dec 11, 2024

View reviewed changes

strseb mentioned this pull request Dec 11, 2024

Allow propertySum to consume any amount of properties #144

Merged

lesleyjanenorton reviewed Dec 12, 2024

View reviewed changes

mcleinman marked this pull request as draft December 12, 2024 23:35

mcleinman added 4 commits December 13, 2024 10:02

PR feedback

677e233

better reuse

afa3aaa

remove unused artifacts

20a6ef7

reuse the constant better

a191fbd

mcleinman mentioned this pull request Dec 13, 2024

Add string for Done button mozilla-l10n/mozilla-vpn-extension-l10n#3

Merged

mcleinman requested review from lesleyjanenorton and strseb December 13, 2024 22:04

mcleinman marked this pull request as ready for review December 13, 2024 22:04

Merge branch 'main' into fxvpn-10-implement-onboarding-flow

dae0fe4

lesleyjanenorton reviewed Dec 13, 2024

View reviewed changes

mcleinman added 2 commits December 13, 2024 14:15

remove unused stuff

6e69e75

happy linter, happy devs

5b2f422

mcleinman requested a review from lesleyjanenorton December 13, 2024 22:35

strseb approved these changes Dec 16, 2024

View reviewed changes

mcleinman added 2 commits December 16, 2024 11:40

Merge branch 'main' into fxvpn-10-implement-onboarding-flow

825cf78

PR feedback

1e90c4b

mcleinman merged commit 1ffa427 into main Dec 16, 2024
4 of 5 checks passed

mcleinman deleted the fxvpn-10-implement-onboarding-flow branch December 16, 2024 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FXVPN-10: onboarding flow #142

FXVPN-10: onboarding flow #142

mcleinman commented Dec 10, 2024 •

edited

Loading

strseb Dec 11, 2024

mcleinman Dec 13, 2024

strseb Dec 11, 2024

mcleinman Dec 13, 2024

strseb Dec 11, 2024

strseb Dec 11, 2024

mcleinman Dec 12, 2024

strseb Dec 11, 2024

strseb Dec 11, 2024

lesleyjanenorton Dec 12, 2024

mcleinman Dec 13, 2024

mcleinman commented Dec 12, 2024

lesleyjanenorton commented Dec 12, 2024

lesleyjanenorton Dec 12, 2024

mcleinman Dec 13, 2024

mcleinman commented Dec 12, 2024

mcleinman commented Dec 13, 2024

lesleyjanenorton Dec 13, 2024

mcleinman Dec 13, 2024

mcleinman Dec 13, 2024

lesleyjanenorton Dec 13, 2024 •

edited

Loading

mcleinman Dec 13, 2024

strseb left a comment

strseb Dec 16, 2024

mcleinman Dec 16, 2024

FXVPN-10: onboarding flow #142

FXVPN-10: onboarding flow #142

Conversation

mcleinman commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcleinman commented Dec 12, 2024

lesleyjanenorton commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcleinman commented Dec 12, 2024

mcleinman commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesleyjanenorton Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strseb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcleinman commented Dec 10, 2024 •

edited

Loading

lesleyjanenorton Dec 13, 2024 •

edited

Loading