Make onboarding optional, able to be ignored on desktop#6564
Make onboarding optional, able to be ignored on desktop#6564franknoirot merged 28 commits intomainfrom
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6564 +/- ##
==========================================
+ Coverage 85.60% 85.63% +0.02%
==========================================
Files 108 108
Lines 47362 47362
==========================================
+ Hits 40546 40558 +12
+ Misses 6816 6804 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bfa0690 to
0d3725f
Compare
0d3725f to
fba9f83
Compare
|
Sorry gotta address something flaky(?) about my new consolidated test. It passes locally on MacOS |
|
The create a new project always is great. I think it is less friction, less complex code on our end to option select all the scenarios of what could be in the system. |
| 6) src/lib/singletons.ts -> src/clientSideScene/sceneEntities.ts -> src/clientSideScene/segments.ts -> src/components/Toolbar/angleLengthInfo.ts | ||
| 7) src/lib/singletons.ts -> src/clientSideScene/sceneEntities.ts -> src/clientSideScene/segments.ts | ||
| 8) src/hooks/useModelingContext.ts -> src/components/ModelingMachineProvider.tsx -> src/components/Toolbar/Intersect.tsx -> src/components/SetHorVertDistanceModal.tsx -> src/lib/useCalculateKclExpression.ts | ||
| 9) src/routes/Onboarding/index.tsx -> src/routes/Onboarding/Camera.tsx -> src/routes/Onboarding/utils.tsx |
| const requestedPath = `${PATHS.FILE}/${encodeURIComponent( | ||
| projectPathWithoutSpecificKCLFile | ||
| )}` | ||
| )}${requestedProjectName.subRoute || ''}` |
|
Found one bug?
I think any of the replay options when it is |
src/routes/Onboarding/utils.tsx
Outdated
| : onboardingRoutes[stepNumber] | ||
| const goToNext = useNextClick(onboardingPaths.INDEX + (nextStep?.path ?? '')) | ||
| : onboardingPathsArray[stepNumber] | ||
| const goToNext = useNextClick(nextStep + onboardingPaths.INDEX) |
There was a problem hiding this comment.
The concatenation of nextStep + onboardingPaths.INDEX may create invalid URL paths since onboardingPaths.INDEX is '/'. When appended to another path, this would result in paths like /camera/ instead of the likely intended /camera. Consider using a path joining utility or template string that properly handles path segments to ensure valid URL construction.
| const goToNext = useNextClick(nextStep + onboardingPaths.INDEX) | |
| const goToNext = useNextClick(nextStep === '' ? onboardingPaths.INDEX : `/${nextStep}`) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
CodSpeed Instrumentation Performance ReportMerging #6564 will not alter performanceComparing Summary
|
6c69d65 to
81aaa97
Compare
Hey @nadr0 sorry I might have gotten the branch in a bad state trying to clear up the circular dependency I had added. I am not experiencing this as of now, would you mind double-checking? |
…ttyCAD/modeling-app into franknoirot/adhoc/optional-onboarding
| // TODO: this is not navigating to the correct `/onboarding/blah` path yet | ||
| navigate( | ||
| makeUrlPathRelative( | ||
| `${PATHS.ONBOARDING.INDEX}${makeUrlPathRelative(onboardingStatus)}` | ||
| ) |
There was a problem hiding this comment.
The nested makeUrlPathRelative calls here are unnecessarily complex and error-prone. Consider using the newly created joinRouterPaths utility function instead, which would handle path construction more reliably:
navigate(joinRouterPaths(PATHS.ONBOARDING.INDEX, onboardingStatus))This would ensure proper path formatting and avoid potential issues with duplicate or missing slashes.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
pierremtb
left a comment
There was a problem hiding this comment.
Didn't test on web but desktop looks great! And so does the code diff
This reverts commit 820082d.
Oversight on my part while refactoring the onboarding system in #6564. The new centralized `acceptOnboarding` workflow constructs a relative path, so we have to get out of the settings before invoking it.
Closes #6542. Makes onboarding tutorial optional on both desktop and web, removing all redirection behavior.
Both platforms
Desktop
Screenshare.-.2025-04-29.9_46_32.AM-compressed.mp4
Browser
Screenshare.-.2025-04-29.9_49_57.AM-compressed.mp4
Tests
I consolidated all tests within onboarding-tests.spec.ts into one longer test. Several of those tests—like the "avatar text" tests—checked behavior that will become irrelevant this week when we update the onboarding to no longer mention all the menus and UI. Some of them were redundant, each checking that the code reset and the model was loaded. This test checks that, clicks through all onboarding steps and back, and verifies the "always create a new project" behavior on desktop in several ways. I left a TODO comment in that test that another test will need to be written for the divergent web onboarding behavior when we support browser E2E tests again.
Other
This PR removes one of our circular dependencies.