Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

feat: Install PostHog and implement data capture on various buttons #1437

Merged
merged 4 commits into from
May 11, 2022

Conversation

chadstewart
Copy link
Contributor

@chadstewart chadstewart commented May 10, 2022

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

This PR [adds] the [feature] posthog logging.

Related Tickets & Documents

Resolves #1430 and resolves #1434

Mobile & Desktop Screenshots/Recordings

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR Compliance Checks

Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.

Watched Files

This pull request modifies specific files that require careful review by the maintainers.

Files Matched

  • npm-shrinkwrap.json
  • package.json

src/index.jsx Outdated
@@ -24,6 +25,8 @@ function Index() {
const [isAdmin, setIsAdmin] = useState(null);

useEffect(() => {
posthog.init('phc_QFGGi71ugzUOXTLUTwMHVTlaHhDBolEC3XNVHNFHKmz', { api_host: 'https://app.posthog.com' });
Copy link
Member

@bdougie bdougie May 10, 2022

Choose a reason for hiding this comment

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

Might need to put this into ENV var POSTHOG_ID

@0-vortex
Copy link
Contributor

For #1430 a very small wrapper could be useful, currently, we init PostHog in index.jsx, and then .capture works everywhere else. To make it easier to catch errors and standardize the way we send events from components, moving that code into something like lib/posthog or lib/analytics would centralize the way we implement PostHog across the app.

Other than that and extracting the environment variable to src/env.d.ts / .env the code LGTM! Would still leave #1430 and do it separately from this PR

@chadstewart
Copy link
Contributor Author

For #1430 a very small wrapper could be useful, currently, we init PostHog in index.jsx, and then .capture works everywhere else. To make it easier to catch errors and standardize the way we send events from components, moving that code into something like lib/posthog or lib/analytics would centralize the way we implement PostHog across the app.

Other than that and extracting the environment variable to src/env.d.ts / .env the code LGTM! Would still leave #1430 and do it separately from this PR

That's a good idea, making the wrapper function. I'll go work on that now.

So the reason why I added both the issues to this PR was because the implement Posthog PR also required implementing data capture on the login button so I figured since I was adding it there, I might as well just add it to the onboarding buttons here rather than make a whole other PR. Figured referencing both issues and also having separate commits would give enough context.

@0-vortex
Copy link
Contributor

For #1430 a very small wrapper could be useful, currently, we init PostHog in index.jsx, and then .capture works everywhere else. To make it easier to catch errors and standardize the way we send events from components, moving that code into something like lib/posthog or lib/analytics would centralize the way we implement PostHog across the app.
Other than that and extracting the environment variable to src/env.d.ts / .env the code LGTM! Would still leave #1430 and do it separately from this PR

That's a good idea, making the wrapper function. I'll go work on that now.

So the reason why I added both the issues to this PR was because the implement Posthog PR also required implementing data capture on the login button so I figured since I was adding it there, I might as well just add it to the onboarding buttons here rather than make a whole other PR. Figured referencing both issues and also having separate commits would give enough context.

I suggested splitting so the wrapper doesn't make the implementation go out of scope, if you want to work on it in this PR that's fine with me, minimal wrapper that logs in, captures event and potential errors; and moving the credentials to the env file should be enough to close both tickets! πŸš€ πŸ•

@chadstewart
Copy link
Contributor Author

chadstewart commented May 10, 2022

I suggested splitting so the wrapper doesn't make the implementation go out of scope, if you want to work on it in this PR that's fine with me, minimal wrapper that logs in, captures event and potential errors; and moving the credentials to the env file should be enough to close both tickets! rocket pizza

Nah, I understand, makes sense. Just feels like this makes the most sense to me honestly.

Wanted to ask a quick question. How does the project work with .envs? I don't see an example of using an env variable outside of some stuff in Vite and I didn't want to go Googling stuff in case there was a particular set up that was there.

@0-vortex
Copy link
Contributor

I suggested splitting so the wrapper doesn't make the implementation go out of scope, if you want to work on it in this PR that's fine with me, minimal wrapper that logs in, captures event and potential errors; and moving the credentials to the env file should be enough to close both tickets! rocket pizza

Nah, I understand, makes sense. Just feels like this makes the most sense to me honestly.

Wanted to ask a quick question. How does the project work with .envs? I don't see an example of using an env variable outside of some stuff in Vite and I didn't want to go Googling stuff in case there was a particular set up that was there.

Sorry for the late answer, https://vitejs.dev/guide/env-and-mode.html should cover everything on the documentation side.

To give specifics, we are using a "dev" local configuration for beta branches and making the git clone work for localhost. The phc_ token should be configured to only work for the selected endpoint in the cloud instance.

Making the phc_ token an environment variable's only purpose is to be able to rotate it across environments, it would still be technically public so the VITE_ notation should be fine, and committing to both env files if you like. This will make a little bit more sense when we apply the code you write in this PR into hot.opensauced πŸ•

Copy link
Member

@bdougie bdougie left a comment

Choose a reason for hiding this comment

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

LGTM

@bdougie
Copy link
Member

bdougie commented May 11, 2022

Making the phc_ token an environment variable's only purpose is to be able to rotate it across environments, it would still be technically public so the VITE_ notation should be fine, and committing to both env files if you like. This will make a little bit more sense when we apply the code you write in this PR into hot.opensauced πŸ•

I would love to also capture this in hot.opensauced

@bdougie bdougie merged commit 9a78906 into main May 11, 2022
@bdougie bdougie deleted the feature-posthog-installation branch May 11, 2022 02:08
github-actions bot pushed a commit that referenced this pull request May 11, 2022
## [0.50.0](v0.49.0...v0.50.0) (2022-05-11)

### πŸ• Features

* Install PostHog and implement data capture on various buttons ([#1437](#1437)) ([9a78906](9a78906))
@github-actions
Copy link
Contributor

πŸŽ‰ This PR is included in version 0.50.0 πŸŽ‰

The release is available on:

Your semantic-release bot πŸ“¦πŸš€

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Post Hog into onboarding buttons Implement PostHog
3 participants