-
Notifications
You must be signed in to change notification settings - Fork 751
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
final "on my own" flow updates #10892
final "on my own" flow updates #10892
Conversation
Build Artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer a cleaner approach here that relied more on internal behaviour within plugins rather than cross plugin redirects.
kolibri/plugins/setup_wizard/assets/src/views/onboarding-forms/SettingUpKolibri.vue
Outdated
Show resolved
Hide resolved
9a88ff3
to
a48084b
Compare
a48084b
to
a4d830b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All seems to be working as intended, just some suggestions for cleanup.
this.search(); | ||
if (window.sessionStorage.getItem(welcomeDimissalKey) !== 'true') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be used directly in the vuex initial state to set the value.
@@ -507,7 +507,12 @@ | |||
}, | |||
}, | |||
created() { | |||
const welcomeDimissalKey = 'DEVICE_WELCOME_MODAL_DISMISSED'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest moving this constant into a common file, maybe in kolibri-common (for reuse across device, learn, and setup wizard plugins).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is called DEVICE_WELCOME_MODAL_DISMISSED not COMMON_... though 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I've actually gone and tested the fix I realize the silliness of my previous comment 😬 😆
@@ -16,17 +23,30 @@ | |||
import NotificationsRoot from 'kolibri.coreVue.components.NotificationsRoot'; | |||
import plugin_data from 'plugin_data'; | |||
import { PageNames } from '../constants'; | |||
import PostSetupModalGroup from '../../../../device/assets/src/views/PostSetupModalGroup.vue'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I'd want to move this to kolibri-common
rather than have so many ..
- but we can also move all this cleanup to a follow up if you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some local testing on this and worked as expected so LGTM, the code looks good as well.
I'll leave the approval to Richard re: the follow-up business moving things to kolibri-common
.
@@ -507,7 +507,12 @@ | |||
}, | |||
}, | |||
created() { | |||
const welcomeDimissalKey = 'DEVICE_WELCOME_MODAL_DISMISSED'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is called DEVICE_WELCOME_MODAL_DISMISSED not COMMON_... though 🤣
@@ -507,7 +507,12 @@ | |||
}, | |||
}, | |||
created() { | |||
const welcomeDimissalKey = 'DEVICE_WELCOME_MODAL_DISMISSED'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I've actually gone and tested the fix I realize the silliness of my previous comment 😬 😆
a4d830b
to
d30b940
Compare
d30b940
to
f6075a8
Compare
ff1da7e
to
171c18a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes make sense, and my suggestions only made things break, so let's move forward with this!
Summary
final updates to "on my own" flow: in concert with recently-merged BE changes, this PR introduces specific behavior for "on my own" users upon exiting the setup wizard - "on my own" users will now be redirected right to
Learn
>Library
and greeted by a welcome message once there (just as they were previously greeted by a welcome modal when they were redirected toDevice
after completing setup).Before - redirected to
Learn
>Home
with no welcome modal(before very recent & related BE changes, would redirect to
Device
with welcome modal)before-learn-home.mov
After - redirected to
Learn
>Library
with customized message in welcome modalafter-learn-library-redirect.mov
References
closes #10760 (in combination with #11019, which is already merged)
Reviewer guidance
Learn
>Library
& greeted by a dismissable welcome modalDevice
for the first time, on subsequent re-visits toLibrary
, or in any other scenarios.Testing checklist
PR process
Reviewer checklist
yarn
andpip
)