Skip to content

Commit

Permalink
Remove omit. Change to back/front instead of top/bottom. Keep the exp…
Browse files Browse the repository at this point in the history 
…anded state array updated with the movement.
  • Loading branch information
@nishasy
nishasy committed Jun 20, 2024
1 parent 8a2da06 commit e79e001
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 63 deletions.
67 changes: 37 additions & 30 deletions packages/perseus-editor/src/components/defining-point-settings.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,36 +17,41 @@ import CoordinatePairInput from "./coordinate-pair-input";
import LabeledSwitch from "./labeled-switch";
import LockedFigureSettingsAccordion from "./locked-figure-settings-accordion";

import type {LockedFigureSettingsCommonProps} from "./locked-figure-settings";
import type {LockedPointType} from "@khanacademy/perseus";

export type Props = Omit<
LockedFigureSettingsCommonProps,
"onMove" | "onRemove"
> &
LockedPointType & {
/**
* Optional label for the point to display in the header summary.
* Defaults to "Point".
*/
label: string;
/**
* Whether the extra point settings are toggled open.
*/
showPoint?: boolean;
/**
* Optional error message to display.
*/
error?: string | null;
/**
* Called when the extra settings toggle switch is changed.
*/
onTogglePoint?: (newValue) => void;
/**
* Called when the props (coords, color, etc.) are updated.
*/
onChangeProps: (newProps: Partial<LockedPointType>) => void;
};
export type Props = LockedPointType & {
/**
* Optional label for the point to display in the header summary.
* Defaults to "Point".
*/
label: string;
/**
* Whether the extra point settings are toggled open.
*/
showPoint?: boolean;
/**
* Optional error message to display.
*/
error?: string | null;
/**
* Called when the extra settings toggle switch is changed.
*/
onTogglePoint?: (newValue) => void;
/**
* Called when the props (coords, color, etc.) are updated.
*/
onChangeProps: (newProps: Partial<LockedPointType>) => void;

// Accordion props
/**
* Whether this accordion is expanded.
*/
expanded?: boolean;
/**
* Called when the accordion is expanded or collapsed.
*/
onToggle?: (expanded: boolean) => void;
};

const DefiningPointSettings = (props: Props) => {
const {
Expand All @@ -58,6 +63,8 @@ const DefiningPointSettings = (props: Props) => {
error,
onChangeProps,
onTogglePoint,
expanded,
onToggle,
} = props;

function handleColorChange(newValue) {
Expand All @@ -66,8 +73,8 @@ const DefiningPointSettings = (props: Props) => {

return (
<LockedFigureSettingsAccordion
expanded={props.expanded}
onToggle={props.onToggle}
expanded={expanded}
onToggle={onToggle}
containerStyle={styles.container}
panelStyle={styles.accordionPanel}
header={
Expand Down
22 changes: 13 additions & 9 deletions packages/perseus-editor/src/components/locked-figure-settings-actions.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ import * as React from "react";

import type {LockedFigureType} from "@khanacademy/perseus";

export type LockedFigureSettingsMovementType = "top" | "up" | "down" | "bottom";
export type LockedFigureSettingsMovementType =
| "back"
| "backward"
| "forward"
| "front";

type Props = {
figureType: LockedFigureType;
Expand Down Expand Up @@ -46,29 +50,29 @@ const LockedFigureSettingsActions = (props: Props) => {
<IconButton
icon={caretDoubleUpIcon}
size="small"
aria-label={`Move locked ${figureType} to the top`}
onClick={() => onMove("top")}
aria-label={`Move locked ${figureType} to the back`}
onClick={() => onMove("back")}
style={styles.iconButton}
/>
<IconButton
icon={caretUpIcon}
size="small"
aria-label={`Move locked ${figureType} up`}
onClick={() => onMove("up")}
aria-label={`Move locked ${figureType} backward`}
onClick={() => onMove("backward")}
style={styles.iconButton}
/>
<IconButton
icon={caretDownIcon}
size="small"
aria-label={`Move locked ${figureType} down`}
onClick={() => onMove("down")}
aria-label={`Move locked ${figureType} forward`}
onClick={() => onMove("forward")}
style={styles.iconButton}
/>
<IconButton
icon={caretDoubleDownIcon}
size="small"
aria-label={`Move locked ${figureType} to the bottom`}
onClick={() => onMove("bottom")}
aria-label={`Move locked ${figureType} to the front`}
onClick={() => onMove("front")}
style={styles.iconButton}
/>
</View>
Expand Down
26 changes: 18 additions & 8 deletions packages/perseus-editor/src/components/locked-figures-section.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,39 +53,49 @@ const LockedFiguresSection = (props: Props) => {
movement: LockedFigureSettingsMovementType,
) {
// Don't allow moving the first figure up or the last figure down.
if (index === 0 && (movement === "top" || movement === "up")) {
if (index === 0 && (movement === "back" || movement === "backward")) {
return;
}
if (
figures &&
index === figures.length - 1 &&
(movement === "down" || movement === "bottom")
(movement === "front" || movement === "forward")
) {
return;
}

const lockedFigures = figures || [];
const newFigures = [...lockedFigures];
const newExpandedStates = [...expandedStates];

// First, remove the figure from its current position.
// First, remove the figure from its current position
// in the figures array and the expanded states array.
const [removedFigure] = newFigures.splice(index, 1);
newExpandedStates.splice(index, 1);

// Then, add it back in the new position.
// Then, add it back in the new position. Add "true" to the
// expanded states array for the new position (it must already
// be open since the button to move it is being pressed from there).
switch (movement) {
case "top":
case "back":
newFigures.unshift(removedFigure);
newExpandedStates.unshift(true);
break;
case "up":
case "backward":
newFigures.splice(index - 1, 0, removedFigure);
newExpandedStates.splice(index - 1, 0, true);
break;
case "down":
case "forward":
newFigures.splice(index + 1, 0, removedFigure);
newExpandedStates.splice(index + 1, 0, true);
break;
case "bottom":
case "front":
newFigures.push(removedFigure);
newExpandedStates.push(true);
break;
}
onChange({lockedFigures: newFigures});
setExpandedStates(newExpandedStates);
}

function removeLockedFigure(index: number) {
Expand Down
32 changes: 16 additions & 16 deletions ...ges/perseus-editor/src/widgets/__tests__/interactive-graph-editor-locked-figures.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ describe("InteractiveGraphEditor locked figures", () => {
color: "red",
};

test(`Calls onChange when a locked ${figureType} is moved top`, async () => {
test(`Calls onChange when a locked ${figureType} is moved back`, async () => {
// Arrange
const onChangeMock = jest.fn();

Expand All @@ -159,7 +159,7 @@ describe("InteractiveGraphEditor locked figures", () => {

// Act
const moveButton = screen.getAllByRole("button", {
name: `Move locked ${figureType} to the top`,
name: `Move locked ${figureType} to the back`,
});
await userEvent.click(moveButton[2]);

Expand All @@ -171,7 +171,7 @@ describe("InteractiveGraphEditor locked figures", () => {
);
});

test(`Calls onChange when a locked ${figureType} is moved up`, async () => {
test(`Calls onChange when a locked ${figureType} is moved backward`, async () => {
// Arrange
const onChangeMock = jest.fn();

Expand All @@ -182,7 +182,7 @@ describe("InteractiveGraphEditor locked figures", () => {

// Act
const moveButton = screen.getAllByRole("button", {
name: `Move locked ${figureType} up`,
name: `Move locked ${figureType} backward`,
});
await userEvent.click(moveButton[2]);

Expand All @@ -194,7 +194,7 @@ describe("InteractiveGraphEditor locked figures", () => {
);
});

test(`Calls onChange when a locked ${figureType} is moved down`, async () => {
test(`Calls onChange when a locked ${figureType} is moved forward`, async () => {
// Arrange
const onChangeMock = jest.fn();

Expand All @@ -205,7 +205,7 @@ describe("InteractiveGraphEditor locked figures", () => {

// Act
const moveButton = screen.getAllByRole("button", {
name: `Move locked ${figureType} down`,
name: `Move locked ${figureType} forward`,
});
await userEvent.click(moveButton[0]);

Expand All @@ -217,7 +217,7 @@ describe("InteractiveGraphEditor locked figures", () => {
);
});

test(`Calls onChange when a locked ${figureType} is moved bottom`, async () => {
test(`Calls onChange when a locked ${figureType} is moved front`, async () => {
// Arrange
const onChangeMock = jest.fn();

Expand All @@ -228,7 +228,7 @@ describe("InteractiveGraphEditor locked figures", () => {

// Act
const moveButton = screen.getAllByRole("button", {
name: `Move locked ${figureType} to the bottom`,
name: `Move locked ${figureType} to the front`,
});
await userEvent.click(moveButton[0]);

Expand All @@ -240,7 +240,7 @@ describe("InteractiveGraphEditor locked figures", () => {
);
});

test(`Does not call onChange when a locked ${figureType} is moved top and is already at the top`, async () => {
test(`Does not call onChange when a locked ${figureType} is moved to the back and is already in the back`, async () => {
// Arrange
const onChangeMock = jest.fn();

Expand All @@ -251,15 +251,15 @@ describe("InteractiveGraphEditor locked figures", () => {

// Act
const moveButton = screen.getAllByRole("button", {
name: `Move locked ${figureType} to the top`,
name: `Move locked ${figureType} to the back`,
});
await userEvent.click(moveButton[0]);

// Assert
expect(onChangeMock).not.toBeCalled();
});

test(`Does not call onChange when a locked ${figureType} is moved up and is already at the top`, async () => {
test(`Does not call onChange when a locked ${figureType} is moved backward and is already in the back`, async () => {
// Arrange
const onChangeMock = jest.fn();

Expand All @@ -270,15 +270,15 @@ describe("InteractiveGraphEditor locked figures", () => {

// Act
const moveButton = screen.getAllByRole("button", {
name: `Move locked ${figureType} up`,
name: `Move locked ${figureType} backward`,
});
await userEvent.click(moveButton[0]);

// Assert
expect(onChangeMock).not.toBeCalled();
});

test(`Does not call onChange when a locked ${figureType} is moved down and is already at the bottom`, async () => {
test(`Does not call onChange when a locked ${figureType} is moved forward and is already in the front`, async () => {
// Arrange
const onChangeMock = jest.fn();

Expand All @@ -289,15 +289,15 @@ describe("InteractiveGraphEditor locked figures", () => {

// Act
const moveButton = screen.getAllByRole("button", {
name: `Move locked ${figureType} down`,
name: `Move locked ${figureType} forward`,
});
await userEvent.click(moveButton[2]);

// Assert
expect(onChangeMock).not.toBeCalled();
});

test(`Does not call onChange when a locked ${figureType} is moved bottom and is already at the bottom`, async () => {
test(`Does not call onChange when a locked ${figureType} is moved to the front and is already in the front`, async () => {
// Arrange
const onChangeMock = jest.fn();

Expand All @@ -308,7 +308,7 @@ describe("InteractiveGraphEditor locked figures", () => {

// Act
const moveButton = screen.getAllByRole("button", {
name: `Move locked ${figureType} to the bottom`,
name: `Move locked ${figureType} to the front`,
});
await userEvent.click(moveButton[2]);

Expand Down

0 comments on commit e79e001

Please sign in to comment.