trent_larson
/
crowd-funder-for-time-pwa
4
1
Fork 2
Code Pull Requests 12 Releases 5 Wiki Activity
Labels Milestones
New Pull Request

Call onboarding dialog only once #144

Open
jose wants to merge 1 commits from onboarding-dialog-fix into master
Conversation 0 Commits 1 Files Changed 1
1 changed files with 12 additions and 10 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 22
      src/views/HomeView.vue

22
src/views/HomeView.vue
View File

@ -478,6 +478,7 @@ export default class HomeView extends Vue {
selectedImageData: Blob | null = null;
isImageViewerOpen = false;
imageCache: Map<string, Blob | null> = new Map();
needsOnboarding = false;
/**
* Initializes the component on mount
@ -603,12 +604,8 @@ export default class HomeView extends Vue {
this.showShortcutBvc = !!settings.showShortcutBvc;
this.isAnyFeedFilterOn = checkIsAnyFeedFilterOn(settings);
// Check onboarding status
if (!settings.finishedOnboarding) {
(this.$refs.onboardingDialog as OnboardingDialog).open(
OnboardPage.Home,
);
}
// Check onboarding status - will be handled in checkOnboarding()
this.needsOnboarding = !settings.finishedOnboarding;
// Check registration status if needed
if (!this.isRegistered && this.activeDid) {
@ -841,11 +838,16 @@ export default class HomeView extends Vue {
* Called by mounted()
*/
private async checkOnboarding() {
let settings = await databaseUtil.retrieveSettingsForActiveAccount();
if (USE_DEXIE_DB) {
settings = await retrieveSettingsForActiveAccount();
// Only check if we haven't already determined onboarding is needed
if (!this.needsOnboarding) {
let settings = await databaseUtil.retrieveSettingsForActiveAccount();
if (USE_DEXIE_DB) {
settings = await retrieveSettingsForActiveAccount();
}
this.needsOnboarding = !settings.finishedOnboarding;
}
if (!settings.finishedOnboarding) {
if (this.needsOnboarding) {
trentlarson commented 2 days ago
Review

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.)
(this.$refs.onboardingDialog as OnboardingDialog).open(OnboardPage.Home);
}
}

Write Preview
Loading…
Cancel
Save