feat: offer event tracking as onboarding option - #37
Conversation
Allows multi-selection; treats empty value as “skip.”
filipmyllari
left a comment
filipmyllari
left a comment
There was a problem hiding this comment.
Just a few comments 😄
| } | ||
|
|
||
| - \`<session recording SDK package name>\``; | ||
| if (!goals.includes('feature-flags')) { |
There was a problem hiding this comment.
Duplicate .env + SDK init entries when event-tracking is selected alone
When goals doesn't include feature-flags, lines 44–47 unconditionally add .env and SDK initialization entries. But if only event-tracking is selected, the template also adds at line 41 — resulting in a vague dep line alongside the specific .env/init entries. More importantly, if goals is ['session-recordings'] alone, you get both the session-recording-specific entry point line and a second generic "added SDK initialization" line, which produces duplicate/confusing "entry point file" entries.
There was a problem hiding this comment.
Yeah, I think it can be completely reworked now; have an idea
There was a problem hiding this comment.
Is 'done' ever reachable now that you removed it from line 21?
There was a problem hiding this comment.
True; phasing is gone, I'll remove everything related to it, thanks!
| function goalPreamble(goals: OnboardingGoal[]): string { | ||
| const labels = goals.map((g) => GOAL_LABELS[g]); | ||
| if (labels.length <= 2) return labels.join(' and '); | ||
| return labels.slice(0, -1).join(', ') + ', and ' + labels.at(-1); | ||
| } | ||
|
|
There was a problem hiding this comment.
Would prob add an empty guard here if values.length somehow comes here as 0 😊
There was a problem hiding this comment.
Conceptually, if it is 0, then the onboarding is considered “skipped,” but I suppose a general check wouldn't hurt still
| <Box marginTop={1}> | ||
| <Text color={Colors.warning}> | ||
| {Icons.star} Event Tracking works best with a managed warehouse or an existing | ||
| warehouse setup. | ||
| </Text> | ||
| </Box> |
There was a problem hiding this comment.
The warehouse note for Event Tracking is always displayed, even on non-browser frameworks that don't show Session Recordings. This is intentional since Event Tracking is in BASE_GOALS, but it means the note renders even if the user doesn't intend to select Event Tracking. Consider showing it conditionally (e.g. only when Event Tracking is toggled on), or if the always-visible note is intentional, this is fine — just flagging as a UX consideration.
There was a problem hiding this comment.
Yeah, sounds reasonable, will add this, thanks!
There was a problem hiding this comment.
Although, on the second thought, having it right away makes is easier to make an informed choice without the need to switch focus when selecting option 🤔
I'd say, let's keep it “as is” for now and if we notice it inconvenient, change it?
2 participants
Uh oh!
There was an error while loading. Please reload this page.