-
Notifications
You must be signed in to change notification settings - Fork 365
[Interactive Graph] Add tangent graph option in the Interactive Graph Editor #3358
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
Changes from all commits
2bc995a
5d0fc99
e10f90d
1555df0
2ae3707
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@khanacademy/perseus": minor | ||
| "@khanacademy/perseus-editor": minor | ||
| --- | ||
|
|
||
| Add tangent graph option in the Interactive Graph Editor |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -369,6 +369,7 @@ class InteractiveGraphEditor extends React.Component<Props> { | |
| this.props.graph?.type ?? | ||
| InteractiveGraph.defaultProps.userInput.type | ||
| } | ||
| apiOptions={this.props.apiOptions} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No action needed: I wish we had a context provider for APIOptions so we didn't have to pipe it through like this... |
||
| // TODO(LEMS-2656): remove TS suppression | ||
| onChange={ | ||
| (( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* We have to use !important until wonder blocks is in the shared layer. */ | ||
| /* TODO(LEMS-3686): Remove the !important once we don't need it anymore. */ | ||
| .tile { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why we need this file. It doesn't seem like any other chart type has a file like this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would align to us using classNames in our styling. This is the effort that @mark-fitzgerald has been leading since last year. |
||
| display: flex !important; | ||
| flex-direction: row !important; | ||
| align-items: center !important; | ||
| background-color: var( | ||
| --wb-semanticColor-core-background-instructive-subtle | ||
| ) !important; | ||
| margin-top: var(--wb-sizing-size_080) !important; | ||
| padding: var(--wb-sizing-size_120) !important; | ||
| border-radius: var(--wb-sizing-size_080) !important; | ||
| } | ||
|
|
||
| .equationSection { | ||
| margin-top: var(--wb-sizing-size_120) !important; | ||
| } | ||
|
|
||
| .equationBody { | ||
| background-color: var( | ||
| --wb-semanticColor-core-background-neutral-subtle | ||
| ) !important; | ||
| border: var(--wb-border-width-thin) solid | ||
| var(--wb-semanticColor-core-border-neutral-subtle) !important; | ||
| margin-top: var(--wb-sizing-size_080) !important; | ||
| padding-left: var(--wb-sizing-size_080) !important; | ||
| padding-right: var(--wb-sizing-size_080) !important; | ||
| font-size: var(--wb-font-size-xSmall) !important; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import {View} from "@khanacademy/wonder-blocks-core"; | ||
| import {Strut} from "@khanacademy/wonder-blocks-layout"; | ||
| import {spacing} from "@khanacademy/wonder-blocks-tokens"; | ||
| import { | ||
| BodyMonospace, | ||
| LabelLarge, | ||
| LabelMedium, | ||
| } from "@khanacademy/wonder-blocks-typography"; | ||
| import * as React from "react"; | ||
|
|
||
| import CoordinatePairInput from "../../../components/coordinate-pair-input"; | ||
|
|
||
| import styles from "./start-coords-tangent.module.css"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels wrong to me - adding styles specifically for the tangent editor. I think we need to rethink this so that we're being consistent across all graph types.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I'm okay with this as I know we've been specifically trying to move to css modules. It makes sense to me that we are breaking the pattern here as we simply haven't updated the old files yet — but I think updating the old files could be a perfect addition to our Interactive Graph: Phase 2 work. Perhaps @mark-fitzgerald has thoughts as well.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good call @SonicScrewdriver i'll add a ticket for that.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| import {getTangentEquation} from "./util"; | ||
|
|
||
| import type {Coord} from "@khanacademy/perseus"; | ||
|
|
||
| type TangentCoords = [Coord, Coord]; | ||
|
|
||
| type Props = { | ||
| startCoords: TangentCoords; | ||
| onChange: (startCoords: TangentCoords) => void; | ||
| }; | ||
|
|
||
| const StartCoordsTangent = (props: Props) => { | ||
| const {startCoords, onChange} = props; | ||
|
|
||
| return ( | ||
| <> | ||
| {/* Current equation */} | ||
| <View className={styles.equationSection}> | ||
| <LabelMedium>Starting equation:</LabelMedium> | ||
| <BodyMonospace className={styles.equationBody}> | ||
| {getTangentEquation(startCoords)} | ||
| </BodyMonospace> | ||
| </View> | ||
|
|
||
| {/* Points UI */} | ||
| <View className={styles.tile}> | ||
| <LabelLarge>Point 1:</LabelLarge> | ||
| <Strut size={spacing.small_12} /> | ||
| <CoordinatePairInput | ||
| coord={startCoords[0]} | ||
| labels={["x", "y"]} | ||
| onChange={(value) => onChange([value, startCoords[1]])} | ||
| /> | ||
| </View> | ||
| <View className={styles.tile}> | ||
| <LabelLarge>Point 2:</LabelLarge> | ||
| <Strut size={spacing.small_12} /> | ||
| <CoordinatePairInput | ||
| coord={startCoords[1]} | ||
| labels={["x", "y"]} | ||
| onChange={(value) => onChange([startCoords[0], value])} | ||
| /> | ||
| </View> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| export default StartCoordsTangent; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { | |
| getQuadraticCoords, | ||
| getSegmentCoords, | ||
| getSinusoidCoords, | ||
| getTangentCoords, | ||
| } from "@khanacademy/perseus"; | ||
| import {UnreachableCaseError} from "@khanacademy/wonder-stuff-core"; | ||
|
|
||
|
|
@@ -64,6 +65,12 @@ export function getDefaultGraphStartCoords( | |
| range, | ||
| step, | ||
| ); | ||
| case "tangent": | ||
| return getTangentCoords( | ||
| {...graph, startCoords: undefined}, | ||
| range, | ||
| step, | ||
| ); | ||
| case "quadratic": | ||
| return getQuadraticCoords( | ||
| {...graph, startCoords: undefined}, | ||
|
|
@@ -93,6 +100,11 @@ export function getDefaultGraphStartCoords( | |
| } | ||
| } | ||
|
|
||
| // TODO(LEMS-3956): Both getSinusoidEquation and getTangentEquation have two | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this TODO also be on |
||
| // formatting quirks: (1) hardcoded "x - " and ") + " produce ugly strings | ||
| // like "x - -0.785" and "+ -1.000" when phase/offset are negative, and | ||
| // (2) no division-by-zero guard when both points share the same x-coordinate. | ||
| // These should be cleaned up together for consistency. | ||
| export const getSinusoidEquation = (startCoords: [Coord, Coord]) => { | ||
| // Get coefficients | ||
| // It's assumed that p1 is the root and p2 is the first peak | ||
|
|
@@ -117,6 +129,30 @@ export const getSinusoidEquation = (startCoords: [Coord, Coord]) => { | |
| ); | ||
| }; | ||
|
|
||
| export const getTangentEquation = (startCoords: [Coord, Coord]) => { | ||
| // Get coefficients | ||
| // It's assumed that p1 is the inflection point and p2 is a quarter-period away | ||
| const p1 = startCoords[0]; | ||
| const p2 = startCoords[1]; | ||
|
|
||
| // Resulting coefficients are canonical for this tangent curve | ||
| const amplitude = p2[1] - p1[1]; | ||
| const angularFrequency = Math.PI / (4 * (p2[0] - p1[0])); | ||
| const phase = p1[0] * angularFrequency; | ||
| const verticalOffset = p1[1]; | ||
|
|
||
| return ( | ||
| "y = " + | ||
| amplitude.toFixed(3) + | ||
| "tan(" + | ||
| angularFrequency.toFixed(3) + | ||
| "x - " + | ||
| phase.toFixed(3) + | ||
| ") + " + | ||
| verticalOffset.toFixed(3) | ||
| ); | ||
| }; | ||
|
|
||
| export const getQuadraticEquation = (startCoords: [Coord, Coord, Coord]) => { | ||
| const p1 = startCoords[0]; | ||
| const p2 = startCoords[1]; | ||
|
Comment on lines
130
to
158
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 Pre-existing issue: Extended reasoning...What the bug is
Why this is pre-existingBoth issues are direct copies of the identical pattern in Proof via concrete exampleFor the formatting issue: given Existing test evidenceThe sinusoid tests in Impact and recommended fixThe impact is purely cosmetic — these equations are only shown in the editor's start coordinates section, not to students. The division-by-zero case requires a content creator to manually type identical x-coordinates into the CoordinatePairInput fields, which is an unusual edge case (interactive dragging prevents it). For comparison,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think this is fine. We can chat with content creators to determine how important this is later. |
||
|
|
@@ -183,15 +219,12 @@ export const shouldShowStartCoordsUI = ( | |
| graph.snapTo !== "sides" | ||
| ); | ||
| case "none": | ||
| case "tangent": | ||
| // TODO(LEMS-3955): return true for tangent once | ||
| // StartCoordsSettingsInner and getDefaultGraphStartCoords | ||
| // handle the tangent type | ||
| return false; | ||
| case "angle": | ||
| case "circle": | ||
| case "linear": | ||
| case "linear-system": | ||
| case "tangent": | ||
| case "quadratic": | ||
| case "ray": | ||
| case "segment": | ||
|
|
||
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.
Nice