Call onboarding dialog only once #144

jose · 2025-07-03T11:18:59Z

jose commented

2025-07-03 11:18:59 +00:00

Removed earlier call to onboarding dialog, choosing to call it only at the end to make way for any error notifications

- Removed earlier call to onboarding dialog, choosing to call it only at the end to make way for any error notifications

jose added 1 commit 2025-07-03 11:18:59 +00:00

Call onboarding dialog only once 3e6e959b19

- Removed earlier call to onboarding dialog, choosing to call it only at the end to make way for any error notifications

trentlarson reviewed 2025-07-03 14:08:20 +00:00

src/views/HomeView.vue

						
				@@ -847,2 +848,3 @@

				    }

				    if (!settings.finishedOnboarding) {

				    if (this.needsOnboarding) {

trentlarson commented

2025-07-03 14:08:20 +00:00

Note that there is a hole in the logic: needsOnboarding is never set to false after they have seen it. (It may not cause a problem because we always recheck, but it does mean that this new variable doesn't always reflect the true state of the user.)

Another possible complication is: after they finish onboarding, they no longer need onboarding, so this method will always check the DB again. (That doesn't seem like it'll cause a problem; I just mention it because I think it's not intended because we keep checking the flag.)

This is a common problem with new state variables. If you ever get a chance to play with functional programming (eg. Haskell, Scheme, LISP) then it'll be interesting how it makes you think differently... and you're able to avoid a lot of state variables in the classes because you always pass all the variables you need.

Anyway, I think it would be fine to eliminate the needsOnboarding flag and always check with retrieveSettingsForActiveAccount in here. That way we don't have to worry about setting a variable. (Another more efficient option is to eliminate that additional DB check here and pass in the flag to the checkOnboarding method from above -- but if it makes things more complicated and harder to understand then it's not worthwhile.)

Note that there is a hole in the logic: needsOnboarding is never set to false after they have seen it. (It may not cause a problem because we always recheck, but it does mean that this new variable doesn't always reflect the true state of the user.) Another possible complication is: after they finish onboarding, they no longer need onboarding, so this method will always check the DB again. (That doesn't seem like it'll cause a problem; I just mention it because I think it's not intended because we keep checking the flag.) This is a common problem with new state variables. If you ever get a chance to play with functional programming (eg. Haskell, Scheme, LISP) then it'll be interesting how it makes you think differently... and you're able to avoid a lot of state variables in the classes because you always pass all the variables you need. Anyway, I think it would be fine to eliminate the needsOnboarding flag and always check with retrieveSettingsForActiveAccount in here. That way we don't have to worry about setting a variable. (Another more efficient option is to eliminate that additional DB check here and pass in the flag to the checkOnboarding method from above -- but if it makes things more complicated and harder to understand then it's not worthwhile.)

anomalist closed this pull request

2025-08-13 05:09:44 +00:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: trent_larson/crowd-funder-for-time-pwa#144