-
Notifications
You must be signed in to change notification settings - Fork 4.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
🪟🧪 [Experiment] Incentivize speedy first connection (aka activation) #17593
🪟🧪 [Experiment] Incentivize speedy first connection (aka activation) #17593
Conversation
@@ -15,4 +15,5 @@ export interface Experiments { | |||
"authPage.oauth.github": boolean; | |||
"authPage.oauth.google.signUpPage": boolean; | |||
"authPage.oauth.github.signUpPage": boolean; | |||
"onbarding.speedyConnection": boolean; |
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.
Launch Darkly FF
@@ -92,7 +95,7 @@ const MainRoutes: React.FC = () => { | |||
<Route path={`${RoutePaths.Settings}/*`} element={<CloudSettingsPage />} /> | |||
<Route path={CloudRoutes.Credits} element={<CreditsPage />} /> | |||
|
|||
{workspace.displaySetupWizard && ( | |||
{(workspace.displaySetupWizard || isExperimentVariant) && ( |
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.
We'll redirect users to the onboarding flow if they click on the banner CTA, therefore we need this route accessible
useTrackPage(PageTrackingCodes.ONBOARDING); | ||
|
||
const navigate = useNavigate(); | ||
|
||
useEffectOnce(() => { |
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.
Not related, but this was duplicated, page view was being tracked by line 64.
@@ -13,7 +14,14 @@ const useStepsConfig = ( | |||
setCurrentStep: (step: StepType) => void; | |||
steps: Array<{ name: JSX.Element; id: StepType }>; | |||
} => { | |||
// exp-speedy-connection | |||
const location = useLocation() as unknown as ILocationState<{ step: StepType }>; |
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.
Need to type location state, as typescript will complain otherwise.
remix-run/history#930
[styles.textDecorationNone]: location.pathname.includes("onboarding"), | ||
})} | ||
to={RoutePaths.Onboarding} | ||
state={{ |
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.
Jump into create source step in the onboarding flow
It would be useful to have more details on the PR description, what this is actually adding. You described why we're doing this, but it would be useful having ideally some screenshots but at least some description like "This will add a banner, before activation, showing a countdown till .... and so on". Right now only the code tells what this PR actually adds. |
const isTrial = Boolean(cloudWorkspace.trialExpiryTimestamp); | ||
const timestamp = localStorage.getItem("exp-speedy-connection-timestamp"); | ||
|
||
const expiredOfferDate = timestamp ? JSON.parse(timestamp) : 0; |
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.
const expiredOfferDate = timestamp ? JSON.parse(timestamp) : 0; | |
const expiredOfferDate = timestamp ? Number(timestamp) : 0; |
We should try to avoid JSON.parse and JSON.strnigify as much as possible. Those methods are really slow, and reduce type-safteness since the result could in theory be anything. If we know we want to parse a string to a number here, using the Number
constructor is the better option.
// exp-speedy-connection | ||
localStorage.setItem( | ||
"exp-speedy-connection-timestamp", | ||
JSON.stringify(new Date(new Date().getTime() + _24_HOURS)) |
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.
Similar to above, just String(...)
instead of JSON.stringify
.
@@ -65,6 +65,8 @@ interface AuthContextApi { | |||
logout: AuthLogout; | |||
} | |||
|
|||
const _24_HOURS = 24 * 60 * 60 * 1000; |
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 we should just inline this instead of adding a _
prefixed variable here.
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 extracted it to make it more descriptive, but can inline instead, no problem!
<FormattedMessage id="experiment.speedyConnection.next" defaultMessage="in the next" />{" "} | ||
</Text> | ||
|
||
<CountDownTimer {...{ expiredOfferDate }} /> |
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.
🎨 codestyle
<CountDownTimer {...{ expiredOfferDate }} /> | |
<CountDownTimer expiredOfferDate={expiredOfferDate} /> |
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.
Curious: is this the case always? Or do we use the first codestyle when there are lots of props? I've found it more useful and faster to code in the past!
@@ -0,0 +1,18 @@ | |||
import { useExperiment } from "hooks/services/Experiment"; |
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.
We're using camelCase for file names, this file name is hyphened.
airbyte-webapp/src/components/experiments/SpeedyConnection/CountDownTimer/use-countdown.ts
Outdated
Show resolved
Hide resolved
<FormattedMessage | ||
id="experiment.speedyConnection.firstConnection" | ||
defaultMessage="Set up your first connection " | ||
/>{" "} |
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.
Messages that contain of several parts, need to be combined in one message (with placeholders) to properly be internationalizationable. I'm happy to sync on that to run you through that.
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.
Happy to learn about the right way to do it, but I think for this case we can go with this version, as message is probably something to be iterated on something to be
The banner currently overlaps also all button on the top. If the banner is shown we need to make sure that the content is pushed below, which happens for the other banner in the div with |
…vation * master: (26 commits) supply a source id for schema discovery in connector integration tests (#17662) Source Iterable: Add permission check for stream (#17602) Moving TrackingClientSingleton.initialize into the bean itself (#17631) Handle null workspace IDs in tracking/reporting methods gracefully (#17641) Bump Airbyte version from 0.40.11 to 0.40.12 (#17653) Revert "Do not wait the end of a reset to return an update (#17591)" (#17640) Standardize HttpRequester's url_base and path format (#17524) Create geography_type enum and add geography column in connection and workspace table (#16818) airbyte-cron: update connector definitions from remote (#16438) Do not wait the end of a reset to return an update (#17591) Remove redundant title labels from connector specs (#17544) Updated GA4 status support large schema discovery (#17394) 🪟 🐛 Fixes connector checks not properly ending their loading state (#17620) 🪟🧪 [Experiment] add hideOnboarding experiment (#17605) Source Recharge: change releaseStage to GA (#17606) Source Recharge: skip stream if 403 received (#17608) remove sonar-scan workflow (#17609) Mark/tables should be full width on all pages (#17401) Auto fail all workfow if there is a Versioning issue (#17562) ...
@timroes ready for re-review |
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.
Reviewed. Left cleanup comments for later. Tested locally seems to work. One issue outstanding that at the moment the banner might show but the route isn't there, which we'll need to address in one of the mentioned ways, after that it looks good to merge.
@use "../../../../scss/variables"; | ||
@use "../../../../scss/colors"; | ||
|
||
.flex { |
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.
🧹 We usually tend to not name CSS rules by their content but the element they are attached too, e.g. this would rather be .container
and the one below .countdownDigits
.
|
||
return ( | ||
<div className={styles.flex}> | ||
<Text as="h2" className={styles.textColorOrange}> |
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.
🧹 Those should usually not be h2
, since they are not really semantically a header to anything in the document. So this currently breaks accessibility.
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.
You are right, changing now.
It was a heading in the Figma, but I should have been more careful
{hours.toString().padStart(2, "0")}h | ||
</Text> | ||
<Text as="h2" className={styles.textColorOrange}> | ||
{minutes.toString().padStart(2, "0")}m{" "} |
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.
Additional space only behind the m
but not the others?
export const useCountdown = (targetDate: string) => { | ||
const countDownDate = new Date(targetDate).getTime(); | ||
|
||
const [countDown, setCountDown] = useState(countDownDate - new Date().getTime()); |
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.
🧹 Date.now()
is usually the preferred one (doesn't require instantiation of an object).
className={classNames(styles.linkCta, { | ||
[styles.textDecorationNone]: location.pathname.includes("onboarding"), | ||
})} | ||
to={RoutePaths.Onboarding} |
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.
🧹 We might want to link later to the Connection page here, depending on how our experiment goes with disabling the onboarding page.
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.
✅ Addressed, so both experiments can live together
const isVariantEnabled = useExperiment("onbarding.speedyConnection", false); | ||
|
||
const isTrial = Boolean(cloudWorkspace.trialExpiryTimestamp); | ||
const timestamp = localStorage.getItem("exp-speedy-connection-timestamp"); |
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.
🧹 We talked about the approach with local storage already offline, so just putting that here to remember.
Also this comes currently with all the drawbacks that this isn't working cross machines and you might even give wrong users credits since we're not resetting that when logging out.
Also we'd usually try to put the key into an exported const some place so we don't need to copy it all over the place.
@@ -25,5 +25,13 @@ $banner-height: 30px; | |||
height: calc(100% - #{$banner-height}); | |||
} | |||
} | |||
|
|||
&.speedyConnectionBanner { | |||
margin-top: 50px; |
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.
🧹 We'd try to extract that in a variable, same as above.
airbyte-webapp/src/packages/cloud/views/layout/MainView/MainView.tsx
Outdated
Show resolved
Hide resolved
@@ -13,7 +14,14 @@ const useStepsConfig = ( | |||
setCurrentStep: (step: StepType) => void; | |||
steps: Array<{ name: JSX.Element; id: StepType }>; | |||
} => { | |||
// exp-speedy-connection | |||
const location = useLocation() as unknown as ILocationState<{ step: StepType }>; |
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.
🧹 We'd usually encapsulate this into a separate hook useLocationState
that just takes a generic, so we don't need to cast in the consuming place to unknown and back, but have that ugly cast limitd to the one place inside the hook.
…vation * master: (32 commits) fixed octavia position and z-index on onboarding page (#17708) Revert "Revert "Do not wait the end of a reset to return an update (#17591)" (#17640)" (#17669) source-google-analytics-v4: use hits metric for check (#17717) Source linkedin-ads: retry 429/5xx when refreshing access token (#17724) 🐛 Source Mixpanel: solve cursor field none expected array (#17699) 🎉 8890 Source MySql: Fix large table issue by fetch size (#17236) Test e2e testing tool commands (#17722) fixed escape character i18n error (#17706) Docs: adds missing " in transformations-with-airbyte.md (#17723) Change Osano token to new project (#17720) Source Github: improve 502 handling for `comments` stream (#17715) #17506 source snapchat marketing: retry failed request for refreshing access token (#17596) MongoDb Source: Increase performance of discover (#17614) Testing tool commands for run scenarios (#17550) Kustomize: Missing NORMALIZATION_JOB_* environment variables in stable-with-resource-limits overlays (#17713) Fix console errors (#17696) Revert: #17047 Airbyte CDK: Improve error for returning non-iterable from connectors parse_response (#17707) #17047 Airbyte CDK: Improve error for returning non-iterable from connectors parse_response (#17626) 📝 Postgres source: document occasional full refresh under cdc mode (#17705) Bump Airbyte version from 0.40.12 to 0.40.13 (#17682) ...
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 LGTM. Left cleanup comments. Testes (the 2nd previous version) locally. Requiring still a green CI, but looks fine for me.
…vation * master: (22 commits) Update full-refresh-append.md (#17784) Update full-refresh-overwrite.md (#17783) Update incremental-append.md (#17785) Update incremental-deduped-history.md (#17786) Update cdc.md (#17787) 🪟 🔧 Ignore classnames during jest snapshot comparison (#17773) feat: replace openjdk with amazoncorretto:17.0.4 on connectors for seсurity compliance (#17511) Start testing buildpulse. (#17712) Add missing types to the registry (#17763) jobs db descriptions (#16543) config db data catalog (#16427) Update lowcode docs (#17752) db migrations to support new webhook operations (#17671) Bump Airbyte version from 0.40.13 to 0.40.14 (#17762) September Release Notes (#17754) Revert "Use java-datadog-tracer-base image (#17625)" (#17759) Add connection migrations for schema changes (#17651) Connection Form Refactor - Part Two (#16748) Improve E2E testing around the Connection Form (#17577) Bump strict encrypt version (#17747) ...
…vation * master: (98 commits) 🐛 Source Bing Ads - Fix Campaigns stream misses Audience and Shopping (#17873) Source S3 - fix schema inference (#17991) 🎉 JDBC sources: store cursor record count in db state (#15535) Introduce webhook configs into workspace api and persistence (#17950) ci: upload test results to github for analysis (#17953) Trigger the connectors build if there are worker changes. (#17976) Add additional sync timing information (#17643) Use page_token_option instead of page_token (#17892) capture metrics around json messages size (#17973) 🐛 Correct kube annotations variable as per the docs. (#17972) 🪟 🎉 Add /connector-builder page with embedded YAML editor (#17482) fix `est_num_metrics_emitted_by_reporter` not being emitted (#17929) Update schema dumps (#17960) Remove the bump in the value.yml (#17959) Ensure database initialization in test container (#17697) Remove typo line from incremental reads docs (#17920) DocS: Update authentication.md (#17931) Use MessageMigration for Source Connection Check. (#17656) fixed links (#17949) remove usages of YamlSeedConfigPersistence (#17895) ...
…irbytehq#17593) * 🪟🧪 [Experiment] Incentivize speedy first connection (aka activation)
What
We know that users that activate, converts better. We want to test if incentivizing users to create their first connection faster (activate) will improve our conversion rate.
To incentivize this behavior, we will be showing a banner with a 24 hour countdown, which will be visible until the user has set up their first connection.
If the user is in the experiment, sees the banner and create her first connection, we'll add (manually for now) 100 credits to their trial
How
<SpeedyConnectionBanner />
responsible to render the banner itself and a couple more components for the banner content, especially the countdown.There is one missing piece, adding orb trial credits when the action is completed. Due to security issues, we can't have that request made directly from the FE as any user would be able to add their own credits in cloud.
For now, we'll do this manually (adding credits on Orb UI) but, since gamification is something we want to invest on, we are drafting a proposal on how this could be automated in the future.