Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/olive-planes-raise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Implementing bug fix for jumping MovableLines in the Correct Answer graph in the editor
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type Props = {
end: boolean;
};
onMovePoint?: (endpointIndex: number, destination: vec.Vector2) => unknown;
onMoveLine?: (delta: vec.Vector2) => unknown;
onMoveLine?: (newStart: vec.Vector2, newEnd: vec.Vector2) => unknown;
};

export const MovableLine = (props: Props) => {
Expand Down Expand Up @@ -110,9 +110,9 @@ export const MovableLine = (props: Props) => {
start={start}
end={end}
extend={extend}
onMove={(delta) => {
onMove={(newStart, newEnd) => {
setAriaLives(["off", "off", "polite"]);
onMoveLine(delta);
onMoveLine(newStart, newEnd);
}}
/>
);
Expand Down Expand Up @@ -141,7 +141,7 @@ type LineProps = {
start: boolean;
end: boolean;
};
onMove: (delta: vec.Vector2) => unknown;
onMove: (newStart: vec.Vector2, newEnd: vec.Vector2) => unknown;
};

const Line = (props: LineProps) => {
Expand Down Expand Up @@ -170,12 +170,19 @@ const Line = (props: LineProps) => {
: undefined;
}

// Capture the offset between start and end at drag begin so we
// can compute absolute positions for both points each frame.
const offsetRef = useRef(vec.sub(end, start));
const line = useRef<SVGGElement>(null);
const {dragging} = useDraggable({
gestureTarget: line,
point: start,
onMove: (newPoint) => {
onMove(vec.sub(newPoint, start));
onMove: (newStart) => {
const newEnd = vec.add(newStart, offsetRef.current);
onMove(newStart, newEnd);
},
onDragStart: () => {
offsetRef.current = vec.sub(end, start);
},
constrainKeyboardMovement: (p) => snap(snapStep, p),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,10 @@ const LinearSystemGraph = (props: LinearSystemGraphProps) => {
}),
}}
ariaDescribedBy={`${linesAriaInfo[i].interceptDescriptionId} ${linesAriaInfo[i].slopeDescriptionId} ${intersectionId}`}
onMoveLine={(delta: vec.Vector2) => {
dispatch(actions.linearSystem.moveLine(i, delta));
onMoveLine={(newStart, newEnd) => {
dispatch(
actions.linearSystem.moveLine(i, newStart, newEnd),
);
}}
extend={{
start: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ const LinearGraph = (props: LinearGraphProps, key: number) => {
ariaLabels={{grabHandleAriaLabel: srLinearGrabHandle}}
ariaDescribedBy={`${interceptDescriptionId} ${slopeDescriptionId}`}
points={line}
onMoveLine={(delta: vec.Vector2) => {
dispatch(actions.linear.moveLine(delta));
onMoveLine={(newStart, newEnd) => {
dispatch(actions.linear.moveLine(newStart, newEnd));
}}
extend={{
start: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ const RayGraph = (props: Props) => {
const {dispatch} = props;
const {coords: line} = props.graphState;

const handleMoveLine = (delta: vec.Vector2) =>
dispatch(actions.ray.moveRay(delta));
const handleMoveLine = (newStart: vec.Vector2, newEnd: vec.Vector2) =>
dispatch(actions.ray.moveRay(newStart, newEnd));
const handleMovePoint = (pointIndex: number, newPoint: vec.Vector2) =>
dispatch(actions.ray.movePoint(pointIndex, newPoint));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,10 @@ const SegmentGraph = ({dispatch, graphState}: SegmentProps) => {
<MovableLine
key={i}
points={segment}
onMoveLine={(delta: vec.Vector2) => {
dispatch(actions.segment.moveLine(i, delta));
onMoveLine={(newStart, newEnd) => {
dispatch(
actions.segment.moveLine(i, newStart, newEnd),
);
}}
onMovePoint={(
endpointIndex: number,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ export const actions = {
moveRadiusPoint,
},
linear: {
moveLine: (delta: vec.Vector2) => moveLine(0, delta),
moveLine: (newStart: vec.Vector2, newEnd: vec.Vector2) =>
moveLine(0, newStart, newEnd),
movePoint: (pointIndex, destination) =>
movePointInFigure(0, pointIndex, destination),
},
Expand Down Expand Up @@ -68,7 +69,8 @@ export const actions = {
movePoint,
},
ray: {
moveRay: (delta: vec.Vector2) => moveLine(0, delta),
moveRay: (newStart: vec.Vector2, newEnd: vec.Vector2) =>
moveLine(0, newStart, newEnd),
movePoint: (pointIndex, destination) =>
movePointInFigure(0, pointIndex, destination),
},
Expand Down Expand Up @@ -109,13 +111,17 @@ export const MOVE_LINE = "move-line";
export interface MoveLine {
type: typeof MOVE_LINE;
itemIndex: number;
delta: vec.Vector2;
newPoints: [vec.Vector2, vec.Vector2];
}
function moveLine(itemIndex: number, delta: vec.Vector2): MoveLine {
function moveLine(
itemIndex: number,
newStart: vec.Vector2,
newEnd: vec.Vector2,
): MoveLine {
return {
type: MOVE_LINE,
itemIndex,
delta,
newPoints: [newStart, newEnd],
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ describe("movePointInFigure", () => {
});

describe("moveSegment", () => {
it("moves an entire segment by the given delta vector", () => {
it("sets segment coords to the given positions", () => {
const state: InteractiveGraphState = {
...baseSegmentGraphState,
coords: [
Expand All @@ -423,7 +423,7 @@ describe("moveSegment", () => {

const updated = interactiveGraphReducer(
state,
actions.segment.moveLine(0, [5, -3]),
actions.segment.moveLine(0, [6, -1], [8, 1]),
);

invariant(updated.type === "segment");
Expand All @@ -446,7 +446,7 @@ describe("moveSegment", () => {

const updated = interactiveGraphReducer(
state,
actions.segment.moveLine(0, [0.5, 0.5]),
actions.segment.moveLine(0, [1.5, 2.5], [3.5, 4.5]),
);

invariant(updated.type === "segment");
Expand All @@ -456,7 +456,7 @@ describe("moveSegment", () => {
]);
});

it("keeps the segment within the graph bounds", () => {
it("clamps points to the graph bounds", () => {
const state: InteractiveGraphState = {
...baseSegmentGraphState,
coords: [
Expand All @@ -469,12 +469,13 @@ describe("moveSegment", () => {

const updated = interactiveGraphReducer(
state,
actions.segment.moveLine(0, [99, 99]),
actions.segment.moveLine(0, [99, 99], [99, 99]),
);

invariant(updated.type === "segment");
// Each point is clamped independently to the graph range
expect(updated.coords[0]).toEqual([
[7, 7],
[9, 9],
[9, 9],
]);
});
Expand All @@ -492,7 +493,7 @@ describe("moveSegment", () => {

const updated = interactiveGraphReducer(
state,
actions.segment.moveLine(0, [1, 1]),
actions.segment.moveLine(0, [2, 3], [4, 5]),
);

expect(updated.hasBeenInteractedWith).toBe(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,27 +331,18 @@ function doMoveLine(
action: MoveLine,
): InteractiveGraphState {
const {snapStep, range} = state;
const [newStart, newEnd] = action.newPoints;
const newLine: PairOfPoints = [
boundAndSnapToGrid(newStart, {snapStep, range}),
boundAndSnapToGrid(newEnd, {snapStep, range}),
];

switch (state.type) {
case "segment":
case "linear-system": {
if (action.itemIndex === undefined) {
throw new Error("Please provide index of line to move");
}
const currentLine = state.coords[action.itemIndex];
// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
if (!currentLine) {
throw new Error("No line to move");
}
const change = getChange(currentLine, action.delta, {
snapStep,
range,
});

const newLine: PairOfPoints = [
snap(snapStep, vec.add(currentLine[0], change)),
snap(snapStep, vec.add(currentLine[1], change)),
];

const newCoords = setAtIndex({
array: state.coords,
index: action.itemIndex,
Expand All @@ -367,17 +358,6 @@ function doMoveLine(
}
case "linear":
case "ray": {
const currentLine = state.coords;
const change = getChange(currentLine, action.delta, {
snapStep,
range,
});

const newLine: PairOfPoints = [
snap(snapStep, vec.add(currentLine[0], change)),
snap(snapStep, vec.add(currentLine[1], change)),
];

return {
...state,
type: state.type,
Expand All @@ -386,8 +366,6 @@ function doMoveLine(
};
}
default:
// The MoveLine action doesn't make sense for other graph types;
// ignore it if it somehow happens
return state;
}
}
Expand Down