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

Hints: fix hints type on PerseusItem to be correct #1489

Merged
merged 6 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
6 changes: 6 additions & 0 deletions .changeset/gentle-planes-rest.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/perseus": patch
"@khanacademy/perseus-editor": patch
---

Correct `hints` type on ItemRenderer
19 changes: 6 additions & 13 deletions packages/perseus-editor/src/hint-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -318,14 +318,13 @@ class CombinedHintsEditor extends React.Component<CombinedHintsEditorProps> {
silent: boolean,
) => {
// TODO(joel) - lens
const hints = _(this.props.hints).clone();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving away from _ gives us better typing.

const hints = [...this.props.hints];
hints[i] = _.extend(
{},
this.serializeHint(i, {keepDeletedWidgets: true}),
newProps,
);

// @ts-expect-error - TS2740 - Type 'Hint' is missing the following properties from type 'readonly Hint[]': length, concat, join, slice, and 18 more.
this.props.onChange({hints: hints}, cb, silent);
};

Expand All @@ -335,23 +334,18 @@ class CombinedHintsEditor extends React.Component<CombinedHintsEditorProps> {
return;
}

const hints = _(this.props.hints).clone();
// @ts-expect-error - TS2339 - Property 'splice' does not exist on type 'Hint'.
const hints = [...this.props.hints];
hints.splice(i, 1);
// @ts-expect-error - TS2322 - Type 'Hint' is not assignable to type 'readonly Hint[]'.
this.props.onChange({hints: hints});
};

handleHintMove: (i: number, dir: number) => void = (
i: number,
dir: number,
) => {
const hints = _(this.props.hints).clone();
// @ts-expect-error - TS2339 - Property 'splice' does not exist on type 'Hint'.
const hints = [...this.props.hints];
const hint = hints.splice(i, 1)[0];
// @ts-expect-error - TS2339 - Property 'splice' does not exist on type 'Hint'.
hints.splice(i + dir, 0, hint);
// @ts-expect-error - TS2322 - Type 'Hint' is not assignable to type 'readonly Hint[]'.
this.props.onChange({hints: hints}, () => {
// eslint-disable-next-line react/no-string-refs
// @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'.
Expand All @@ -360,10 +354,9 @@ class CombinedHintsEditor extends React.Component<CombinedHintsEditorProps> {
};

addHint: () => void = () => {
const hints = _(this.props.hints)
.clone()
// @ts-expect-error - TS2339 - Property 'concat' does not exist on type 'Hint'.
.concat([{content: ""}]);
const hints = [...this.props.hints].concat([
{content: "", images: {}, widgets: {}},
]);
jeremywiebe marked this conversation as resolved.
Show resolved Hide resolved
this.props.onChange({hints: hints}, () => {
const i = hints.length - 1;
// eslint-disable-next-line react/no-string-refs
Expand Down
2 changes: 1 addition & 1 deletion packages/perseus/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ export type {
DomInsertCheckFn,
EditorMode,
FocusPath,
Hint,
ImageDict,
ImageUploader,
JiptLabelStore,
Expand All @@ -167,6 +166,7 @@ export type {
} from "./types";
export type {ParsedValue} from "./util";
export type {
Hint,
LockedFigure,
LockedFigureColor,
LockedFigureFillType,
Expand Down
11 changes: 10 additions & 1 deletion packages/perseus/src/perseus-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export type PerseusItem = {
// The details of the question being asked to the user.
question: PerseusRenderer;
// A collection of hints to be offered to the user that support answering the question.
hints: ReadonlyArray<PerseusRenderer>;
hints: ReadonlyArray<Hint>;
// Details about the tools the user might need to answer the question
answerArea: PerseusAnswerArea | null | undefined;
// Multi-item should only show up in Test Prep content and it is a variant of a PerseusItem
Expand Down Expand Up @@ -127,6 +127,15 @@ export type PerseusRenderer = {
};
};

export type Hint = PerseusRenderer & {
/**
* When `true`, causes the previous hint to be replaced with this hint when
* displayed. When `false`, the previous hint remains visible when this one
* is displayed.
*/
replace?: boolean;
};
Copy link
Member

Choose a reason for hiding this comment

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

The PerseusRenderer type already has a replace property — should that be removed now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! Yes, we can remove it now!


export type PerseusImageDetail = {
// The width of the image
width: number;
Expand Down
6 changes: 1 addition & 5 deletions packages/perseus/src/types.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import type {ILogger} from "./logging/log";
import type {Item} from "./multi-items/item-types";
import type {
Hint,
PerseusAnswerArea,
PerseusRenderer,
PerseusWidget,
PerseusWidgetsMap,
} from "./perseus-types";
Expand Down Expand Up @@ -47,10 +47,6 @@ export type PerseusScore =
message?: string | null | undefined;
};

export type Hint = PerseusRenderer & {
replace?: boolean;
};

export type Version = {
major: number;
minor: number;
Expand Down