-
Notifications
You must be signed in to change notification settings - Fork 351
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
Feature: Analytics #606
Feature: Analytics #606
Conversation
## Summary: This PR introduces a new React Context for plumbing dependencies through Perseus. The context uses a new type `PerseusDependenciesV2` (so it doesn't conflict with the original `PerseusDependencies`). This PR also provides a React hook (`useDependencies`) for accessing dependencies. This hook will commonly be used in wrapper component to attach dependencies to the wrapped component (next PR). I also renamed `sendEvent` to `onAnalyticsEvent` as that feels more natural for a callback (especially event callback... a la 'onClick`). This causes a bit more churn but hopefully that's ok as we're early in the analytics journey. Issue: LC-950 ## Test plan: `yarn test` `yarn tsc` Author: jeremywiebe Reviewers: nedredmond, eamspoker, jeremywiebe, handeyeco Required Reviewers: Approved By: nedredmond Checks: ✅ finish_coverage, ✅ finish_coverage, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ gerald, ✅ gerald, ⏭ Publish npm snapshot, ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ gerald Pull Request URL: #600
## Summary: The next step in the new dependencies implementation. In this PR we add a new `dependencies` prop to the top-level renderers and provide that dependency object to children in the render tree via the `DependenciesContext`. I've also hooked the `Expression` widget to these new dependencies to prove out the concept. Finally, this PR includes providing the dependencies in Storybook and Cypress as the types required it now. _💡**Note**: This PR is easier to review if you turn on the "Hide whitespace" option in the "Files changed" tab._ <img width="200" alt="image" src="https://github.com/Khan/perseus/assets/77138/feb8f9e0-0e63-49d4-9ba9-b1b1cde5d53a"> Issue: LC-950 ## Test plan: `yarn tsc` `yarn test` Author: jeremywiebe Reviewers: jeremywiebe, nedredmond, eamspoker, kevinbarabash, handeyeco Required Reviewers: Approved By: nedredmond Checks: ✅ finish_coverage, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ gerald, ✅ finish_coverage, ⏭ Publish npm snapshot, ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ finish_coverage, ✅ gerald, ✅ gerald, ⏭ Publish npm snapshot, ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ gerald Pull Request URL: #601
## Summary: This PR introduces a Story for the ArticleRender. We had one example in a Definition widget story, but now there's a Story for the ArticleRenderer in the Renderers area. <img width="600" alt="image" src="https://github.com/Khan/perseus/assets/77138/1dbd34e7-d2de-491a-9204-d0a61c174a0c"> Issue: "none" ## Test plan: `yarn storybook` and navigate to **Perseus** >> **Renderers** >> **Article Renderer** Author: jeremywiebe Reviewers: nedredmond, SonicScrewdriver, handeyeco, eamspoker Required Reviewers: Approved By: nedredmond, SonicScrewdriver Checks: ✅ finish_coverage, ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ gerald, ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ finish_coverage, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ gerald, ✅ Jest Coverage (ubuntu-latest, 16.x) Pull Request URL: #602
…tions type for prop types (#603) ## Summary: This PR should wrap up the analytics work for now. We remove the (now unused) `analytics` key on the legacy `PerseusDependencies` object/type. It also does a bit of tweaking of the `RenderProps` for the Expression widget by moving it to the top of the file with the other prop types. Issue: LC-950 ## Test plan: `yarn tsc` `yarn test` Author: jeremywiebe Reviewers: nedredmond, SonicScrewdriver, eamspoker, handeyeco Required Reviewers: Approved By: nedredmond, SonicScrewdriver Checks: ✅ finish_coverage, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ gerald Pull Request URL: #603
🦋 Changeset detectedLatest commit: 8ba69c1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
Size Change: +586 B (0%) Total Size: 846 kB
ℹ️ View Unchanged
|
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've already approved the pieces. ✅ for the whole!
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (9044495) and published it to npm. You Example: yarn add @khanacademy/perseus@PR606 |
7bfa2f9
to
3dbb8f8
Compare
3dbb8f8
to
2e1e3ce
Compare
…es for it (#625) ## Summary: I've been working to get a proper type for the `trackInteraction` `APIOption`. I've migrated the `InteractionTracker` to be generic so that it only accepts extra arguments defined by the `WidgetProps<T,T,T>` (this means each widget can call `trackInteraction` with args it defines). In the `APIOptions` the `trackInteraction` callback now defines a union of all extra args that all widgets provide. Issue: "none" ## Test plan: Build, build flow types, and then test it in webapp. Author: jeremywiebe Reviewers: jeremywiebe, nedredmond, handeyeco, kevinbarabash, SonicScrewdriver Required Reviewers: Approved By: handeyeco Checks: ✅ finish_coverage, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ Lint, Typecheck, and Test (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ gerald Pull Request URL: #625
## Summary: As I was integrating Perseus analytics in webapp, it became really difficult to work out how to provide the virtualKeypadVersion consistently when Perseus wasn't always providing it. After some thought, I figured out that we can pretty-reliably derive the keypad version. Given that we are actively moving to coalesce on a single keypad, I hope this is an acceptable approach for the short term. When we get to one keypad, we can delete this `virtualKeypadVersion` from the analytics anyways. Issue: LC-950 ## Test plan: ✅ `yarn typecheck` ✅ `yarn test` Author: jeremywiebe Reviewers: nedredmond, SonicScrewdriver, handeyeco Required Reviewers: Approved By: nedredmond Checks: ✅ finish_coverage, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 16.x), ✅ Cypress Coverage (ubuntu-latest, 16.x), ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Check for .changeset file (ubuntu-latest, 16.x), ✅ gerald Pull Request URL: #684
This PR represents the set of PRs to implement a new dependency system for Perseus and the first usage: analytics.
I'm creating a PR for landing this feature branch so that I can get the automatic npm snapshots so that I can start integrating into webapp to work out any kinks, before landing all of this code onto
main
.