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

Move default editor props for NumberLine #2147

Merged
merged 5 commits into from
Jan 24, 2025
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/yellow-baboons-fly.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/perseus-core": minor
"@khanacademy/perseus-editor": patch
---

Move upgrade logic for NumberLine to Perseus Core
2 changes: 2 additions & 0 deletions packages/perseus-core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,7 @@ export {pluck, mapObject} from "./utils/objective_";

export {default as expressionLogic} from "./widgets/expression";
export type {ExpressionDefaultWidgetOptions} from "./widgets/expression";
export {default as numberLineLogic} from "./widgets/number-line";
export type {NumberLineDefaultWidgetOptions} from "./widgets/number-line";

export type * from "./widgets/logic-export.types";
44 changes: 44 additions & 0 deletions packages/perseus-core/src/widgets/number-line/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import type {PerseusNumberLineWidgetOptions} from "../../data-schema";
import type {WidgetLogic} from "../logic-export.types";

export type NumberLineDefaultWidgetOptions = Pick<
PerseusNumberLineWidgetOptions,
| "range"
| "labelRange"
| "labelStyle"
| "labelTicks"
| "divisionRange"
| "numDivisions"
| "snapDivisions"
| "tickStep"
| "correctRel"
| "correctX"
| "initialX"
| "showTooltip"
>;

const defaultWidgetOptions: NumberLineDefaultWidgetOptions = {
range: [0, 10],

labelRange: [null, null],
labelStyle: "decimal",
labelTicks: true,

divisionRange: [1, 12],
numDivisions: 5,
snapDivisions: 2,

tickStep: null,
correctRel: "eq",
correctX: null,
initialX: null,

showTooltip: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this was originally showTooltips, but in PerseusNumberLineWidgetOptions that's showTooltip. I think this might have been a bug of some sort due to lack of typing? So I was hoping someone could double check my thinking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The prop that the NumberLine component receives is showTooltips though, and the plural form is used in the React code, so my thought is that the data schema and other instances that reference showTooltip are wrong.

image image

I grepped through the entire content bucket and I can't find instances of number-line using the singular showTooltip either.

@benchristel I see the parser uses singular, so I wonder if because it's marked as optional(), it succeeded (ie. it never saw a value, but passed because it's optional). We should fix the parser here too.

};

const numberLineWidgetLogic: WidgetLogic = {
name: "number-line",
defaultWidgetOptions,
};

export default numberLineWidgetLogic;
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ describe("number-line-editor", () => {
screen.getByRole("checkbox", {name: "Show tooltips"}),
);

expect(onChangeMock).toBeCalledWith({showTooltips: true});
expect(onChangeMock).toBeCalledWith({showTooltip: true});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nervous about this too

});

it("should be possible to update tick steps", async () => {
Expand Down
69 changes: 27 additions & 42 deletions packages/perseus-editor/src/widgets/number-line-editor.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
/* eslint-disable @khanacademy/ts-no-error-suppressions */
import {number as knumber} from "@khanacademy/kmath";
import {components, EditorJsonify} from "@khanacademy/perseus";
import {
numberLineLogic,
type NumberLineDefaultWidgetOptions,
} from "@khanacademy/perseus-core";
import {Checkbox} from "@khanacademy/wonder-blocks-form";
import PropTypes from "prop-types";
import * as React from "react";
import _ from "underscore";

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

const {ButtonGroup, InfoTip, NumberInput, RangeInput} = components;

type Range = [number, number];
Expand All @@ -15,52 +20,32 @@ const bound = (x: number, gt: number, lt: number): number =>

const EN_DASH = "\u2013";

type Props = any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I was concerned with the type, I converted PropTypes to TS


class NumberLineEditor extends React.Component<Props> {
static propTypes = {
range: PropTypes.arrayOf(PropTypes.number).isRequired,

labelRange: PropTypes.arrayOf(PropTypes.number).isRequired,
labelStyle: PropTypes.string.isRequired,
labelTicks: PropTypes.bool,
type Props = {
range: number[];

divisionRange: PropTypes.arrayOf(PropTypes.number).isRequired,
numDivisions: PropTypes.number.isRequired,
snapDivisions: PropTypes.number,
labelRange: ReadonlyArray<number>;
labelStyle: string;
labelTicks: boolean;

tickStep: PropTypes.number,
correctRel: PropTypes.oneOf(["lt", "gt", "le", "ge", "eq"]),
correctX: PropTypes.number,
initialX: PropTypes.number,
isTickCtrl: PropTypes.bool,
divisionRange: ReadonlyArray<number>;
numDivisions: number;
snapDivisions: number;

onChange: PropTypes.func.isRequired,
tickStep: number;
correctRel: "lt" | "gt" | "le" | "ge" | "eq";
correctX: number;
initialX: number;
isTickCtrl?: boolean;

static: PropTypes.bool,
showTooltips: PropTypes.bool,
};
static?: boolean;
showTooltip: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note this should be plural.

} & Changeable.ChangeableProps;

class NumberLineEditor extends React.Component<Props> {
static widgetName = "number-line" as const;

static defaultProps: Props = {
range: [0, 10],

labelRange: [null, null],
labelStyle: "decimal",
labelTicks: true,

divisionRange: [1, 12],
numDivisions: 5,
snapDivisions: 2,

tickStep: null,
correctRel: "eq",
correctX: null,
initialX: null,

showTooltips: false,
};
static defaultProps: NumberLineDefaultWidgetOptions =
numberLineLogic.defaultWidgetOptions;

onRangeChange: (arg1: Range) => void = (range) => {
// Changing the range constrains the initial position, as well as the
Expand Down Expand Up @@ -389,9 +374,9 @@ class NumberLineEditor extends React.Component<Props> {
{!this.props.static && (
<Checkbox
label="Show tooltips"
checked={this.props.showTooltips}
checked={this.props.showTooltip}
onChange={(value) => {
this.props.onChange({showTooltips: value});
this.props.onChange({showTooltip: value});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes me nervous.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rightly. :) It should be singular as noted above. :)

}}
/>
)}
Expand Down
Loading