New event logging to Interactive Graph. #1719
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
catandthemachines
requested review from
jeremywiebe,
benchristel and
nicolecomputer
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (24fb584) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1719If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1719 |
|
Size Change: +79 B (+0.01%) Total Size: 865 kB
ℹ️ View Unchanged
|
| const [width, height] = props.box; | ||
| const tickStep = props.step as vec.Vector2; | ||
|
|
||
| const uniqueId = React.useId(); | ||
| const descriptionId = `interactive-graph-description-${uniqueId}`; | ||
| const graphRef = React.useRef<HTMLElement>(null); | ||
| const dependencies = useDependencies(); |
There was a problem hiding this comment.
A slight stylistic improvement that might make it easier for future readers to see what parts of dependencies are being used:
| const dependencies = useDependencies(); | |
| const {analytics} = useDependencies(); |
| payload: { | ||
| type: type, | ||
| }, |
There was a problem hiding this comment.
suggestion: We can shorten this shorthand property naming:
| payload: { | |
| type: type, | |
| }, | |
| payload: { type }, |
| export type InteractiveGraphName = | ||
| | "angle" | ||
| | "segment" | ||
| | "linear-system" | ||
| | "linear" | ||
| | "ray" | ||
| | "none" | ||
| | "polygon" | ||
| | "point" | ||
| | "circle" | ||
| | "quadratic" | ||
| | "sinusoid"; | ||
|
|
||
| export type InteractiveGraphEnum = | ||
| | "ANGLE" | ||
| | "SEGMENT" | ||
| | "LINEAR_SYSTEM" | ||
| | "LINEAR" | ||
| | "RAY" | ||
| | "NONE" | ||
| | "POLYGON" | ||
| | "POINT" | ||
| | "CIRCLE" | ||
| | "QUADRATIC" | ||
| | "SINUSOID"; | ||
|
|
There was a problem hiding this comment.
suggestion: I'm guessing the InteracativeGraphEnum matches the enum values for the event in CEDAR? In that case, I'd recommend that this mapping live in webapp's adapter rather than here. Otherwise we're pulling CEDAR concepts into the Perseus codebase. I think it's completely fine to use the interactive-graph's graph type in the event and then map it to the CEDAR value in webapp.
Summary:
Adding new analytic event to interactive graph when it renders while adding details on what type of graph is rendered.
Issue: LEMS-2324
Test plan: