Skip to content
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

Create UI for adding a locked point to interactive graph editor (mafs only) #1074

Merged
merged 8 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
2 changes: 2 additions & 0 deletions packages/perseus-editor/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"@khanacademy/wonder-blocks-form": "^4.4.5",
"@khanacademy/wonder-blocks-i18n": "^2.0.2",
"@khanacademy/wonder-blocks-icon": "4.0.1",
"@khanacademy/wonder-blocks-icon-button": "^5.1.13",
"@khanacademy/wonder-blocks-spacing": "^4.0.1",
"@khanacademy/wonder-blocks-tokens": "^1.1.0",
"@khanacademy/wonder-blocks-typography": "^2.1.10",
Expand All @@ -65,6 +66,7 @@
"@khanacademy/wonder-blocks-form": "^4.4.5",
"@khanacademy/wonder-blocks-i18n": "^2.0.2",
"@khanacademy/wonder-blocks-icon": "4.0.1",
"@khanacademy/wonder-blocks-icon-button": "^5.1.13",
"@khanacademy/wonder-blocks-spacing": "^4.0.1",
"@khanacademy/wonder-blocks-tokens": "^1.1.0",
"@khanacademy/wonder-blocks-typography": "^2.1.10",
Expand Down
8 changes: 6 additions & 2 deletions packages/perseus-editor/src/components/labeled-row.tsx
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The click target was taking up the whole row when it wasn't supposed to. Figured I'd fix it really quickly while I was here.

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {View} from "@khanacademy/wonder-blocks-core";
import {spacing} from "@khanacademy/wonder-blocks-tokens";
import {LabelSmall} from "@khanacademy/wonder-blocks-typography";
import {StyleSheet} from "aphrodite";
import {StyleSheet, css} from "aphrodite";
import * as React from "react";

import type {StyleType} from "@khanacademy/wonder-blocks-core";
Expand All @@ -17,7 +17,7 @@ const LabeledRow = (props: {
const {children, label, labelSide = "left", style} = props;

return (
<label>
<label className={css(styles.label)}>
<View style={[styles.row, style]}>
{labelSide === "start" || (
<LabelSmall style={styles.spaceEnd}>{label}</LabelSmall>
Expand All @@ -32,10 +32,14 @@ const LabeledRow = (props: {
};

const styles = StyleSheet.create({
label: {
width: "fit-content",
},
row: {
flexDirection: "row",
marginTop: spacing.xSmall_8,
alignItems: "center",
width: "fit-content",
},
spaceStart: {
marginInlineStart: spacing.xSmall_8,
Expand Down
42 changes: 42 additions & 0 deletions packages/perseus-editor/src/components/locked-figure-select.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import {View} from "@khanacademy/wonder-blocks-core";
import {ActionItem, ActionMenu} from "@khanacademy/wonder-blocks-dropdown";
import {spacing, color} from "@khanacademy/wonder-blocks-tokens";
import {StyleSheet} from "aphrodite";
import * as React from "react";

type Props = {
id: string;
onChange: (value: string) => void;
};

const LockedFigureSelect = (props: Props) => {
const {id, onChange} = props;

return (
<View style={styles.container}>
<ActionMenu menuText="Add element" style={styles.addElementSelect}>
{[
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This needs the brackets because there's only one child. We can get rid of this after adding more types.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:til: I would have used a fragment here.

<ActionItem
key={`${id}-point`}
label="Point"
onClick={() => onChange("point")}
>
Point
</ActionItem>,
]}
</ActionMenu>
</View>
);
};

const styles = StyleSheet.create({
container: {
marginTop: spacing.medium_16,
},
addElementSelect: {
backgroundColor: color.fadedBlue16,
borderRadius: spacing.xxxSmall_4,
},
});

export default LockedFigureSelect;
19 changes: 19 additions & 0 deletions packages/perseus-editor/src/components/locked-figure-settings.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import * as React from "react";

import LockedPointSettings from "./locked-point-settings";

import type {Props as LockedPointProps} from "./locked-point-settings";

// Union this type with other locked figure types when they are added.
type Props = LockedPointProps;

const LockedFigureSettings = (props: Props) => {
switch (props.type) {
case "point":
return <LockedPointSettings {...props} />;
}

return null;
};

export default LockedFigureSettings;
82 changes: 82 additions & 0 deletions packages/perseus-editor/src/components/locked-figures-section.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import {View, useUniqueIdWithMock} from "@khanacademy/wonder-blocks-core";
import {spacing} from "@khanacademy/wonder-blocks-tokens";
import {StyleSheet} from "aphrodite";
import * as React from "react";

import LockedFigureSelect from "./locked-figure-select";
import LockedFigureSettings from "./locked-figure-settings";
import {getDefaultFigureForFigureType} from "./util";

import type {Props as InteractiveGraphEditorProps} from "../widgets/interactive-graph-editor";
import type {LockedFigure, LockedFigureType} from "@khanacademy/perseus";

type Props = {
figures?: Array<LockedFigure>;
onChange: (props: Partial<InteractiveGraphEditorProps>) => void;
};

const LockedFiguresSection = (props: Props) => {
const uniqueId = useUniqueIdWithMock().get("locked-figures-section");
const {figures, onChange} = props;

function addLockedFigure(newFigure: LockedFigureType) {
const lockedFigures = figures || [];
const newProps = {
lockedFigures: [
...lockedFigures,
getDefaultFigureForFigureType(newFigure),
],
};
onChange(newProps);
}

function removeLockedFigure(index: number) {
const lockedFigures = figures || [];
onChange({
lockedFigures: [
...lockedFigures.slice(0, index),
...lockedFigures.slice(index + 1),
],
});
}

function changeCoord(index: number, coord: [number, number]) {
const lockedFigures = figures || [];
const newProps = {
lockedFigures: [
...lockedFigures.slice(0, index),
{
...lockedFigures[index],
coord,
},
...lockedFigures.slice(index + 1),
],
};
onChange(newProps);
}

return (
<View style={styles.container}>
{figures?.map((figure, index) => (
<LockedFigureSettings
key={`${uniqueId}-locked-${figure}-${index}`}
{...figure}
onRemove={() => removeLockedFigure(index)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a possibly-unfounded concern that using indexes for these callbacks could be problematic. I know for react, you aren't supposed to use array indexes for keys. Does that apply here?

onChangeCoord={(coord) => changeCoord(index, coord)}
/>
))}
<LockedFigureSelect
id={`${uniqueId}-select`}
onChange={addLockedFigure}
/>
</View>
);
};

const styles = StyleSheet.create({
container: {
paddingTop: spacing.large_24,
},
});

export default LockedFiguresSection;
125 changes: 125 additions & 0 deletions packages/perseus-editor/src/components/locked-point-settings.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
import {View, useUniqueIdWithMock} from "@khanacademy/wonder-blocks-core";
import {TextField} from "@khanacademy/wonder-blocks-form";
import IconButton from "@khanacademy/wonder-blocks-icon-button";
import {Spring} from "@khanacademy/wonder-blocks-layout";
import {color, spacing} from "@khanacademy/wonder-blocks-tokens";
import {LabelMedium, LabelLarge} from "@khanacademy/wonder-blocks-typography";
import trashIcon from "@phosphor-icons/core/bold/trash-bold.svg";
import {StyleSheet} from "aphrodite";
import * as React from "react";

import {getValidNumberFromString} from "./util";

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

export type Props = LockedPoint & {
onRemove: () => void;
onChangeCoord: (coord: [number, number]) => void;
};

const LockedPointSettings = (props: Props) => {
const {coord, onRemove, onChangeCoord} = props;
const [coordState, setCoordState] = React.useState([
// Using strings to make it easier to work with the text fields.
coord[0].toString(),
coord[1].toString(),
]);

// Generate unique IDs so that the programmatic labels can be associated
// with their respective text fields.
const ids = useUniqueIdWithMock();
const xCoordId = ids.get("x-coord");
const yCoordId = ids.get("y-coord");

function handleBlur() {
const validCoord = [
getValidNumberFromString(coordState[0]),
getValidNumberFromString(coordState[1]),
] as [number, number];

// Make the text field only show valid numbers after blur.
setCoordState([validCoord[0].toString(), validCoord[1].toString()]);
// Update the graph with the new coordinates.
onChangeCoord(validCoord);
}

function handleChange(newValue, coordIndex) {
const newCoord = [...coordState];
newCoord[coordIndex] = newValue;
setCoordState(newCoord);
}

return (
<View style={styles.container}>
{/* Title row */}
<View style={styles.row}>
<LabelLarge>Point</LabelLarge>
<Spring />
<IconButton
icon={trashIcon}
aria-label={`Delete locked point at ${coordState[0]}, ${coordState[1]}`}
onClick={onRemove}
/>
</View>

{/* Coordinates */}
<View>
<View style={styles.row}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: From a design perspective, I wonder if we should put the x and y coord inputs on the same row. Vertical space is so precious in our editors and so I think we should try to avoid scrolling if at all possible.

What do you think about something like this:

*Point* 
Coordinate [x ] [y ]

Where the inputs would have a watermark/placeholder for which axis they're for? I think authors using this editor would understand "x" always comes first so even with data in the fields they'd likely understand it's an x, y coordinate pair.

<LabelMedium
htmlFor={xCoordId}
style={styles.label}
tag="label"
>
x Coordinate
</LabelMedium>
<TextField
id={xCoordId}
value={coordState[0]}
onChange={(newValue) => handleChange(newValue, 0)}
onBlur={handleBlur}
style={styles.textField}
/>
</View>

<View style={styles.row}>
<LabelMedium
htmlFor={yCoordId}
style={styles.label}
tag="label"
>
y Coordinate
</LabelMedium>
<TextField
id={yCoordId}
value={coordState[1]}
onChange={(newValue) => handleChange(newValue, 1)}
onBlur={handleBlur}
style={styles.textField}
/>
</View>
</View>
</View>
);
};

const styles = StyleSheet.create({
container: {
backgroundColor: color.fadedBlue8,
marginBottom: spacing.xSmall_8,
padding: spacing.medium_16,
borderRadius: spacing.xSmall_8,
},
row: {
flexDirection: "row",
alignItems: "center",
marginBottom: spacing.xSmall_8,
},
label: {
marginInlineEnd: spacing.xSmall_8,
},
textField: {
width: spacing.xxxLarge_64,
},
});

export default LockedPointSettings;
24 changes: 24 additions & 0 deletions packages/perseus-editor/src/components/util.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
import type {
LockedFigure,
LockedFigureType,
LockedPoint,
} from "@khanacademy/perseus";

export function focusWithChromeStickyFocusBugWorkaround(element: Element) {
// NOTE(marksandstrom) Chrome sticky focus bug.
//
Expand Down Expand Up @@ -36,3 +42,21 @@ export function focusWithChromeStickyFocusBugWorkaround(element: Element) {
// @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'Element'.
element.focus({preventScroll: true});
}

export function getValidNumberFromString(value: string) {
const parsed = parseInt(value);
// If the value is not a number, return 0.
return isNaN(parsed) ? 0 : parsed;
}

export function getDefaultFigureForFigureType(
type: LockedFigureType,
): LockedFigure {
switch (type) {
case "point":
return {
type: "point",
coord: [0, 0],
} as LockedPoint;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ export default {
},
},

lockedFigures: {
control: {
type: "object",
},
type: {
name: "Array<LockedFigure>",
required: false,
},
},

onChange: {
control: {
type: "function",
Expand Down
Loading
Loading