-
Notifications
You must be signed in to change notification settings - Fork 854
fix: Improve small details on tutorials #7320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4be122c
3383eaf
8ab0ffe
05099b8
2033ded
b5029f9
635bb20
0bcacc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,26 +5,86 @@ | |
| import { goto } from '$app/navigation' | ||
| import { sendUserToast, type ToastAction } from '$lib/toast' | ||
| import { getLocalSetting, storeLocalSetting } from '$lib/utils' | ||
| import { skipAllTodos, syncTutorialsTodos } from '$lib/tutorialUtils' | ||
| import { | ||
| skipAllTodos, | ||
| syncTutorialsTodos, | ||
| TUTORIAL_BANNER_DISMISSED_KEY | ||
| } from '$lib/tutorialUtils' | ||
| import { tutorialsToDo, userStore } from '$lib/stores' | ||
| import { TUTORIALS_CONFIG } from '$lib/tutorials/config' | ||
| import { hasRoleAccess } from '$lib/tutorials/roleUtils' | ||
| import { onMount } from 'svelte' | ||
|
|
||
| const DISMISSED_KEY = 'tutorial_banner_dismissed' | ||
| let isDismissed = $state(false) | ||
|
|
||
| onMount(() => { | ||
| // Check if banner has been dismissed | ||
| isDismissed = getLocalSetting(DISMISSED_KEY) === 'true' | ||
| /** | ||
| * Get all tutorial indexes that are accessible to the current user based on their role. | ||
| * Automatically recomputes when $userStore changes. | ||
| */ | ||
| const accessibleTutorialIndexes = $derived.by(() => { | ||
| const indexes = new Set<number>() | ||
| const user = $userStore | ||
|
|
||
| for (const tab of Object.values(TUTORIALS_CONFIG)) { | ||
| // Check if user has access to this tab category | ||
| if (!hasRoleAccess(user, tab.roles)) { | ||
| continue | ||
| } | ||
|
|
||
| for (const tutorial of tab.tutorials) { | ||
| // Check if tutorial has an index and user has access to it | ||
| if (tutorial.index !== undefined && hasRoleAccess(user, tutorial.roles)) { | ||
| indexes.add(tutorial.index) | ||
| } | ||
| } | ||
| } | ||
| return indexes | ||
| }) | ||
|
|
||
| onMount(async () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential Issue: This onMount(async () => {
try {
await syncTutorialsTodos()
// ... rest of logic
} catch (error) {
console.error('Failed to sync tutorial progress:', error)
// Fallback to manual dismissal check only
isDismissed = getLocalSetting(DISMISSED_KEY) === 'true'
}
})This prevents the banner from being stuck in a loading state if the API call fails. |
||
| try { | ||
| // Sync tutorial progress from backend first | ||
| await syncTutorialsTodos() | ||
|
|
||
| // Check if banner has been manually dismissed | ||
| const manuallyDismissed = getLocalSetting(TUTORIAL_BANNER_DISMISSED_KEY) === 'true' | ||
|
|
||
| // Safe to check tutorialsToDo here since we awaited syncTutorialsTodos() above | ||
| // Filter tutorialsToDo to only include tutorials accessible to the user | ||
| const remainingAccessibleTutorials = $tutorialsToDo.filter((index) => | ||
| accessibleTutorialIndexes.has(index) | ||
| ) | ||
|
|
||
| // Check if all accessible tutorials are completed | ||
| const allTutorialsCompleted = remainingAccessibleTutorials.length === 0 | ||
|
|
||
| // Dismiss banner if manually dismissed OR all accessible tutorials completed | ||
| if (manuallyDismissed || allTutorialsCompleted) { | ||
| isDismissed = true | ||
| // Set localStorage when all tutorials are completed to persist dismissal | ||
| // Note: This will re-set the key on every page load if localStorage is cleared | ||
| // but tutorials remain completed in backend. This is intentional - the banner | ||
| // should stay hidden if tutorials are completed, regardless of localStorage state. | ||
| if (allTutorialsCompleted) { | ||
| storeLocalSetting(TUTORIAL_BANNER_DISMISSED_KEY, 'true') | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to sync tutorial progress:', error) | ||
| // Fallback to manual dismissal check only if API call fails | ||
| isDismissed = getLocalSetting(TUTORIAL_BANNER_DISMISSED_KEY) === 'true' | ||
| } | ||
| }) | ||
|
|
||
| async function handleSkipAllTutorials() { | ||
| await skipAllTodos() | ||
| await syncTutorialsTodos() | ||
| storeLocalSetting(DISMISSED_KEY, 'true') | ||
| storeLocalSetting(TUTORIAL_BANNER_DISMISSED_KEY, 'true') | ||
| isDismissed = true | ||
| } | ||
|
|
||
| function dismissBanner() { | ||
| storeLocalSetting(DISMISSED_KEY, 'true') | ||
| storeLocalSetting(TUTORIAL_BANNER_DISMISSED_KEY, 'true') | ||
| isDismissed = true | ||
|
|
||
| const actions: ToastAction[] = [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,9 +274,7 @@ | |
| {/if} | ||
| </PageHeader> | ||
|
|
||
| {#if !$userStore?.operator} | ||
| <TutorialBanner /> | ||
| {/if} | ||
| <TutorialBanner /> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentional Change - Operators Can Now See Banner: This removes the check that was hiding the tutorial banner from operators. Question: Is this intentional? Looking at the commit message "Allow new operator so see the tutorial banner", it seems intentional, but worth clarifying:
If this is intentional to show operators the banner (perhaps they have their own tutorials), this is fine. Otherwise, you might want to keep the conditional but move the role logic to inside |
||
|
|
||
| {#if !$userStore?.operator} | ||
| <div class="w-full overflow-auto scrollbar-hidden pb-2"> | ||
|
|
||
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.
Svelte 5 Best Practice: Consider using
$derivedfor this computation since it depends on reactive state ($userStore). This would make the reactivity more explicit and efficient:const accessibleTutorialIndexes = $derived.by(() => { const indexes = new Set<number>() const user = $userStore for (const tab of Object.values(TUTORIALS_CONFIG)) { if (!hasRoleAccess(user, tab.roles)) { continue } for (const tutorial of tab.tutorials) { if (tutorial.index !== undefined && hasRoleAccess(user, tutorial.roles)) { indexes.add(tutorial.index) } } } return indexes })This way the set is automatically recomputed when
$userStorechanges, and you can reference it directly inonMountwithout calling a function.