Skip to content
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

🪟 🔧 Add Segment event to invite users modal #18862

Merged

Conversation

letiescanciano
Copy link
Contributor

@letiescanciano letiescanciano commented Nov 2, 2022

Demo: https://www.loom.com/share/1e899c9678124321a62dc71d23bfe52e

What

Adding a new Airbyte.UI.User.Invite Segment event to be able to track user invites across sources, destinations and access management pages.

How

Add new event
Add tracking on invite

@letiescanciano letiescanciano requested a review from a team as a code owner November 2, 2022 17:42
@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 2, 2022
@@ -19,7 +19,9 @@ export const useInviteUsersModalService = () => {
return ctx;
};

export const InviteUsersModalServiceProvider: React.FC<React.PropsWithChildren<unknown>> = ({ children }) => {
export const InviteUsersModalServiceProvider: React.FC<
React.PropsWithChildren<unknown> & { invitedFrom: "source" | "destination" | "user.settings" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to always exclude properties into their own interface, i.e. as a InviteUsersModalServiceProviderProps. Also that interface, should be used as the generic type for PropsWithChildren instead of intersected with it, i.e. React.FC<React.PropsWithChildren<InviteUsersModalServiceProviderProps>>

Copy link
Contributor Author

@letiescanciano letiescanciano Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make the change 😊
Could you explain why we prefer to/should use the interface as the generic type?
I'm super interested in learning more about Typescript as I'm fairly new to it and don't usually use more than basics.

Copy link
Collaborator

@timroes timroes Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The React.PropsWithChildren's sole purpose is basically to wrap the properties and add children to those props, so doing React.PropsWithChildren<unknown> & { ... } is kind of like saying: I have the perfect template for a shopping list at home, and when going to the shop I take that (empty) template and my actual list ("Manolitos, Manolitos, and more Manolitos") that I wrote on a completely blank second paper. (I'll let you know once I come up with a better analogy.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great analogy! Totally understood! thanks for taking the time to explain it 😊

Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment about code style that should be addressed. Other than that looks good to me. Have not tested locally.

@letiescanciano letiescanciano temporarily deployed to more-secrets November 4, 2022 08:06 Inactive
@letiescanciano letiescanciano force-pushed the analytics/add-tracking-to-invite-users-experiment branch from 909fdc0 to 1af5dbd Compare November 4, 2022 08:13
@letiescanciano letiescanciano temporarily deployed to more-secrets November 4, 2022 08:15 Inactive
@letiescanciano letiescanciano merged commit bf58536 into master Nov 4, 2022
@letiescanciano letiescanciano deleted the analytics/add-tracking-to-invite-users-experiment branch November 4, 2022 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform team/growth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants