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.

Sign up for GitHub

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

Add readOnly mode to interactive graphs to prevent keyboard interactions when question marked correct #1487

Merged
merged 10 commits into from
Aug 8, 2024
Merged

Add readOnly mode to interactive graphs to prevent keyboard interactions when question marked correct #1487

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a4b90fe
Add readOnly mode to interactive graphs
Myranae Aug 5, 2024
2392dae
Merge branch 'refs/heads/main' into tb/LEMS-2175/keyboard-cant-contro…
Myranae Aug 5, 2024
480bc97
Add changeset
Myranae Aug 5, 2024
a2edade
Add missing readOnly property
Myranae Aug 5, 2024
4cd2605
Add readOnly graph story
Myranae Aug 5, 2024
09c5df0
Add a comment for readOnly property
Myranae Aug 5, 2024
ff6d524
Refactor and remove duplicated code
Myranae Aug 6, 2024
b2fafd4
Consolidate hint mode and readOnly mode at config
Myranae Aug 6, 2024
f415144
Add a test confirming readOnly stops interaction
Myranae Aug 7, 2024
5685692
Merge branch 'refs/heads/main' into tb/LEMS-2175/keyboard-cant-contro…
Myranae Aug 8, 2024
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/short-paws-suffer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Make graphs non-interactive via keyboard when their question has been answered correctly
4 changes: 4 additions & 0 deletions packages/perseus/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,10 @@ export type APIOptions = Readonly<{
) => unknown;
GroupMetadataEditor?: React.ComponentType<StubTagEditorType>;
showAlignmentOptions?: boolean;
/**
* A boolean that indicates whether the associated problem has been
* answered correctly and should no longer be interactive.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for documenting this!

readOnly?: boolean;
answerableCallback?: (arg1: boolean) => unknown;
getAnotherHint?: () => unknown;
Expand Down
20 changes: 20 additions & 0 deletions packages/perseus/src/widgets/__stories__/interactive-graph.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,17 @@ const mafsOptions = {
},
};

const polygonReadOnlyOptions = {
apiOptions: {
readOnly: true,
flags: {
mafs: {
polygon: true,
},
},
},
};

type StoryArgs = Record<any, any>;

export const SideBySideFlipbook = (args: StoryArgs): React.ReactElement => (
Expand Down Expand Up @@ -71,6 +82,15 @@ export const PolygonWithMafs = (args: StoryArgs): React.ReactElement => (
<RendererWithDebugUI {...mafsOptions} question={polygonQuestion} />
);

export const PolygonWithMafsReadOnly = (
args: StoryArgs,
): React.ReactElement => (
<RendererWithDebugUI
{...polygonReadOnlyOptions}
question={polygonQuestion}
/>
);
Copy link
Member

Choose a reason for hiding this comment

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

Optional suggestion: I wonder if we could make this more skimmable and save some duplicated code by refactoring to something like this:

export const PolygonWithMafsReadOnly = (
    args: StoryArgs,
): React.ReactElement => (
    <RendererWithDebugUI
        apiOptions={{...enableMafs, readOnly: true}}
        question={polygonQuestion}
    />
);

where enableMafs would be defined like this:

const enableMafs: ApiOptions = {
    flags: {
        mafs: {
            segment: true,
            polygon: true,
            // ... other flags as needed ...
        },
    },
}

The other stories could be refactored to use a similar pattern. That way, we wouldn't have to have the mafs flags defined in two places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Love it! I tried to refactor the {...mafsOptions} part, but wasn't quite sure how to do it. I think I might have been missing the double brackets. 🤦‍♀️ Thank you for pointing this out! Updated :)


export const Ray = (args: StoryArgs): React.ReactElement => (
<RendererWithDebugUI question={rayQuestion} />
);
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/interactive-graph.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1827,6 +1827,7 @@ class InteractiveGraph extends React.Component<Props, State> {
box={box}
showTooltips={!!this.props.showTooltips}
hintMode={this.props.hintMode}
readOnly={this.props.apiOptions?.readOnly}
/>
);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/perseus/src/widgets/interactive-graphs/graphs/circle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function MovableCircle(props: {
onMove: (newCenter: vec.Vector2) => unknown;
}) {
const {center, radius, onMove} = props;
const {snapStep, hintMode} = useGraphConfig();
const {snapStep, hintMode, readOnly} = useGraphConfig();
Copy link
Member

Choose a reason for hiding this comment

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

Since both hintMode and readOnly both come from the graph config and are always used together, what do you think about consolidating them? Creating the graph config would look like this:

<GraphConfigContext.Provider
  value={{
    // ...
    disableKeyboardInteraction: hintMode || readOnly,
  }}
>

And then individual components would just look at disableKeyboardInteraction. That way, the || logic wouldn't have to be duplicated in every component.

Copy link
Contributor Author

@Myranae Myranae Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was also considering this. Consolidating some of these modes didn't seem like it was the direction we were going, so I wasn't sure something like this would be the right option. I love this though, so definitely will update. Hint mode disables all interaction though, right? Since readOnly also disables mouse interaction (just not as a result of this setting though), why don't we call it disableInteraction? Ooo, or is it called disableKeyboardInteraction because we use it for tabIndex?

Copy link
Member

Choose a reason for hiding this comment

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

I don't recall exactly where the mouse interactions get disabled. If the code that disables them is reading from useGraphConfig() to decide what to do, then disableInteraction sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll double check :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like mouse interactions are disabled just below where we set the config. Doesn't look like it's reading from the config, so will stick with disableKeyboardInteractions.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! 😁


const draggableRef = useRef<SVGGElement>(null);

Expand All @@ -63,7 +63,7 @@ function MovableCircle(props: {
return (
<g
ref={draggableRef}
tabIndex={hintMode ? -1 : 0}
tabIndex={hintMode || readOnly ? -1 : 0}
className={`movable-circle ${dragging ? "movable-circle--dragging" : ""}`}
>
<ellipse
Expand Down
8 changes: 4 additions & 4 deletions packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-line.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ function useControlPoint(
color: string | undefined,
onMovePoint: (newPoint: vec.Vector2) => unknown,
) {
const {snapStep, hintMode} = useGraphConfig();
const {snapStep, hintMode, readOnly} = useGraphConfig();
const [focused, setFocused] = useState(false);
const keyboardHandleRef = useRef<SVGGElement>(null);
useDraggable({
Expand All @@ -100,7 +100,7 @@ function useControlPoint(
<g
data-testid="movable-point__focusable-handle"
className="movable-point__focusable-handle"
tabIndex={hintMode ? -1 : 0}
tabIndex={hintMode || readOnly ? -1 : 0}
ref={keyboardHandleRef}
onFocus={() => setFocused(true)}
onBlur={() => setFocused(false)}
Expand Down Expand Up @@ -142,7 +142,7 @@ export const Line = (props: LineProps) => {
const {start, end, onMove, extend, stroke = defaultStroke} = props;

const [startPtPx, endPtPx] = useTransformVectorsToPixels(start, end);
const {range, graphDimensionsInPixels, snapStep, hintMode} =
const {range, graphDimensionsInPixels, snapStep, hintMode, readOnly} =
useGraphConfig();

let startExtend: vec.Vector2 | undefined = undefined;
Expand Down Expand Up @@ -172,7 +172,7 @@ export const Line = (props: LineProps) => {
<>
<g
ref={line}
tabIndex={hintMode ? -1 : 0}
tabIndex={hintMode || readOnly ? -1 : 0}
className="movable-line"
data-testid="movable-line"
style={{cursor: dragging ? "grabbing" : "grab"}}
Expand Down
5 changes: 3 additions & 2 deletions packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point-view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ const hitboxSizePx = 48;
// the description of https://github.com/Khan/perseus/pull/1240
export const MovablePointView = forwardRef(
(props: Props, hitboxRef: ForwardedRef<SVGGElement>) => {
const {range, markings, showTooltips, hintMode} = useGraphConfig();
const {range, markings, showTooltips, hintMode, readOnly} =
useGraphConfig();
const {
point,
color = WBColor.blue,
Expand Down Expand Up @@ -90,7 +91,7 @@ export const MovablePointView = forwardRef(
className={pointClasses}
style={{"--movable-point-color": color, cursor} as any}
data-testid="movable-point"
tabIndex={hintMode ? -1 : tabIndex(focusBehavior)}
tabIndex={hintMode || readOnly ? -1 : tabIndex(focusBehavior)}
>
<circle
className="movable-point-hitbox"
Expand Down
4 changes: 2 additions & 2 deletions packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const PolygonGraph = (props: Props) => {
const {dispatch} = props;
const {coords, showAngles, showSides, range, snapStep, snapTo} =
props.graphState;
const {hintMode} = useGraphConfig();
const {hintMode, readOnly} = useGraphConfig();

// TODO(benchristel): can the default set of points be removed here? I don't
// think coords can be null.
Expand Down Expand Up @@ -103,7 +103,7 @@ export const PolygonGraph = (props: Props) => {
color="transparent"
svgPolygonProps={{
ref,
tabIndex: hintMode ? -1 : 0,
tabIndex: hintMode || readOnly ? -1 : 0,
strokeWidth: TARGET_SIZE,
style: {
cursor: dragging ? "grabbing" : "grab",
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ function getBaseMafsGraphProps(): MafsGraphProps {
showTooltips: false,
showProtractor: false,
hintMode: false,
readOnly: false,
labels: ["x", "y"],
dispatch: () => {},
state: {
Expand Down
4 changes: 3 additions & 1 deletion packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,11 @@ export type MafsGraphProps = {
state: InteractiveGraphState;
dispatch: React.Dispatch<InteractiveGraphAction>;
hintMode: boolean;
readOnly: boolean;
};

export const MafsGraph = (props: MafsGraphProps) => {
const {state, dispatch, labels, hintMode} = props;
const {state, dispatch, labels, hintMode, readOnly} = props;
const [width, height] = props.box;
const tickStep = props.step as vec.Vector2;
return (
Expand All @@ -66,6 +67,7 @@ export const MafsGraph = (props: MafsGraphProps) => {
height,
labels,
hintMode,
readOnly,
}}
>
<View
Expand Down
2 changes: 2 additions & 0 deletions packages/perseus/src/widgets/interactive-graphs/reducer/use-graph-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export type GraphConfig = {
height: number; // pixels
labels: readonly string[];
hintMode?: boolean;
readOnly?: boolean;
};

const defaultGraphConfig: GraphConfig = {
Expand All @@ -33,6 +34,7 @@ const defaultGraphConfig: GraphConfig = {
height: 0,
labels: [],
hintMode: false,
readOnly: false,
};

export const GraphConfigContext =
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ function getBaseStatefulMafsGraphProps(): StatefulMafsGraphProps {
showTooltips: false,
showProtractor: false,
hintMode: false,
readOnly: false,
labels: ["x", "y"],
graph: {type: "segment"},
};
Expand Down
1 change: 1 addition & 0 deletions packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export type StatefulMafsGraphProps = {
showProtractor: boolean;
labels: InteractiveGraphProps["labels"];
hintMode: boolean;
readOnly: boolean;
};

// Rather than be tightly bound to how data was structured in
Expand Down