-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Onboarding improvements #1723
Onboarding improvements #1723
Conversation
Hm, small bug to fix - after the first onboarding and getting an event, it redirected me back to the start of the onboarding... so not yet ready for review :) |
Ok, that's fixed! |
Something was still broken in the build, fixed now.
|
Hey @mariusandra the reason for reloading the page instead of just doing an API call again is that if you change the SSL/TLS configuration, the check won't change unless the page is fully reloaded. |
@paolodamico ah, got it! I reverted this. |
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.
Review mostly from a UX perspective. Just minor comments.
-
I would change the
/onboarding/*
URLs to something likesources/*
oringestion/*
because: a) we may reuse this same flow for already onboarded users that wish to add additional event ingestion sources in a guided way; and b) we might implement a different flow for the user onboarding at some point in the future (e.g. a feature walk-through). This way we can future-proof (mostly the metrics / analytics). Side note: It's great that you split this into its separate URL and each step now has a different URL, analytics on this was a bit painful before. -
Small UI thing, based on the background color of the notification, I would maybe change the text to white or more on the light side for better readability.
-
I would move the
/onboarding/mobile/api
out of themobile
space as this is relevant for server-side events too (which we'll implement on New guided event ingestion #1547) [same for/onboarding/web-custom/api
].- The copy to clipboard here copies the entire thing, but it's not a cURL command, so it's not pretty pretty helpful to copy the whole thing, I would either just copy the JSON body or turn this snippet into a cURL command.
-
Should we maybe do a universal verify/listening endpoint instead of
/onboarding/web-custom/nodejs/verify
,/onboarding/mobile/verify
? I think it might be easier to do analytics this way.
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 looks good. @paolodamico tag teamed the manual QA
Hey @paolodamico ! Thank you for the UX comments! I'll make changes soon, just to clarify a few things:
Thoughts? Are these changes/tradeoffs worth it? Better to use a |
In reply to @mariusandra: I think the best way would be to do the static routes with QS params ( |
Hey! I fixed all the feedback, except for renaming |
@paolodamico ready for another look |
Changes
This converts the onboarding page to TypeScript, converts the logic of changing card panes to Kea and fixes the regular JS code snippet, which was cut after the first line.
All of this works, though it's still a WIP as I want to implement changing the URL when you move between tabs, so we can use the back/forward buttons to go back without restarting everything. That was the original aim of these improvements. This should be easy to accomplish via the
urlToAction
andactionToUrl
logic keys, pending slight alteration to thepath
system (using selectors to create the ordered steps beforehand based on existing input, so that theindex
will just follow the precalculated array)I also saw under certain conditions the steps go "step 1 of 3", "step 3 of 4", "step 4 of 4" (or something like that, can't remember 100%).
Checklist