Call onboarding dialog only once #144

Open

jose wants to merge 1 commits from onboarding-dialog-fix into master

jose commented 3 weeks ago

Owner

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 3 weeks ago

3e6e959b19 Call onboarding dialog only once

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

trentlarson reviewed 3 weeks ago

src/views/HomeView.vue

												
					    }
				
					    if (!settings.finishedOnboarding) {
				
					    if (this.needsOnboarding) {

trentlarson commented 3 weeks ago

Poster

Owner

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

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No Assignees

2 Participants

Notifications

Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.