-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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] Show speedy connection banner only to corporate emails #19354
🪟 🧪 [Experiment] Show speedy connection banner only to corporate emails #19354
Conversation
@@ -0,0 +1,3752 @@ | |||
export const FREE_EMAIL_SERVICE_PROVIDERS = [ |
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 added this list of free email providers so we can filter more than gmail.com
@@ -170,6 +172,9 @@ export const AuthenticationProvider: React.FC<React.PropsWithChildren<unknown>> | |||
hasPasswordLogin(): boolean { | |||
return !!state.providers?.includes("password"); | |||
}, | |||
hasCorporateEmail(): 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.
Added this new function here so it can be used across the whole app because I feel we will start segmenting more and more experiments only for corporate emails.
Let me know if this is not the right place or there’s a better one
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 that might be a valid place for it. Alternatively we would put it into src/utils
into a utility function, but I think this can live on the AuthService
as well.
@@ -44,7 +35,6 @@ export const SpeedyConnectionBanner = () => { | |||
timer: () => <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.
Just a note that I noticed while testing: I'd suggest to remove the seconds from this counter. Having something ticking down every second of the top of the page feels really destracting from what you want to do on the page.
background-color: colors.$blue-100; | ||
color: colors.$dark-blue; |
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.
The banner design was made by Nico for all our banners. I'd prefer if we don't diverge for this banner from it. In general 🧹 if we keep this, we should just use the AlertBanner
component.
@@ -7,7 +7,7 @@ | |||
} | |||
|
|||
.countDownTimerItem { | |||
color: colors.$orange; | |||
color: colors.$blue-400; |
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.
Sorry I missed that when changing the design, only changed the banner colors.
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.
No problem!
airbyte-webapp/src/packages/cloud/services/auth/AuthService.tsx
Outdated
Show resolved
Hide resolved
78541e5
to
b659afe
Compare
@timroes changes applied! |
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. Have tested locally, seems to work as expected.
…ls (#19354) * 🪟 🧪 [Experiment] Show speedy connection banner only to corporate emails * PR comments * update banner styles to match design. Remove seconds Co-authored-by: Tim Roes <tim@airbyte.io>
What
Quick update on Speedy Connection experiment before launching.
🎬 https://www.loom.com/share/198587330904445cbd38c8a9298c4c52