-
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
Fix to event details fired from interactive graph. #1723
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (50461b4) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1723 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1723 |
Size Change: +25 B (0%) Total Size: 866 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.
Thank you! LGTM :)
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.
@catandthemachines Sorry that I missed this before you landed it. We need to fix the values passed to widgetType
and widgetId
:)
widgetType: "INTERACTIVE_GRAPH", | ||
widgetId: "interactive-graph", |
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.
Oh, this isn't right. The widgetType
should be interactive-graph
(unfortunately, Perseus uses name
and widgetType
in different places, they're the same concept) and widgetId
is typically something like interactive-graph 1
.
It looks like we pass down the widgetId
(which is a part of the WidgetProps
type.
We don't define that prop on the StatefulMafsGraphProps
type, but it's definitely passed down.
We'll need to update this to pass the actual widgetId
so that our analytics data is correct.
Summary:
Missing two addtional event details fired from interactive graph
Issue: LEMS-2324
Test plan:
N/A