Added logic to convertDotToTimes for the v2 keypad. #722

SonicScrewdriver · 2023-09-12T17:22:06Z

Summary:

Ensured that we're getting the convertDotToTimes value from content's "times" so that we can display the intended multiplication symbol in the new v2 keypad. I also updated some variable names to maintain some consistency.

Issue: LC-1092

Test plan:

Created new tests to ensure that the button changes as desired.

changeset-bot · 2023-09-12T17:22:09Z

🦋 Changeset detected

Latest commit: babda8b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@khanacademy/math-input	Major
@khanacademy/perseus	Minor
@khanacademy/perseus-editor	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

khan-actions-bot · 2023-09-12T17:22:23Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/lucky-bees-bow.md, testing/render-keypad-with-cypress.tsx, packages/math-input/src/full-math-input.stories.tsx, packages/math-input/src/index.ts, packages/math-input/src/types.ts, packages/perseus/src/widgets/expression.tsx, packages/math-input/src/components/keypad/keypad-button.tsx, packages/math-input/src/components/keypad/keypad-mathquill.stories.tsx, packages/math-input/src/components/keypad/keypad.stories.tsx, packages/math-input/src/components/keypad/keypad.tsx, packages/math-input/src/components/keypad/mobile-keypad.tsx, packages/math-input/src/components/keypad/shared-keys.tsx, packages/perseus/src/widgets/__stories__/expression.stories.tsx, packages/math-input/src/components/keypad/__tests__/keypad-v2-mathquill.test.tsx, packages/math-input/src/components/keypad/__tests__/keypad.test.tsx, packages/math-input/src/components/keypad/__tests__/__snapshots__/keypad.test.tsx.snap

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2023-09-12T17:24:22Z

npm Snapshot: Published

🎉 Good news!! We've packaged up the latest commit from this PR (e3c5646) and published it to npm. You
can install it using the tag PR722.

Example:

yarn add @khanacademy/perseus@PR722

SonicScrewdriver · 2023-09-12T17:25:21Z

packages/math-input/src/components/keypad/keypad-mathquill.stories.tsx

@@ -78,7 +78,7 @@ export function V2KeypadWithMathquill() {
                            basicRelations
                            divisionKey
                            logarithms
-                            multiplicationDot
+                            convertDotToTimes


The three variable names sounded confusing as they were: convertDotToTimes, multiplicationDot, and times. multiplicationDot seems like it would technically be the inverse of the original variable logic (times), which was causing a lot of confusion when trying to test for the correct settings. I've renamed multiplicationDot in order to maintain readability.

I'm sure you're fine doing this, but I want to confirm that we're not mixing up concepts. In my head:

convertDotToTimes meant "when adding things to the input, convert the symbol 'dot' to be 'x'" - so doing the conversion from the perspective of the input.

multiplicationDot/times meant "in the keypad, show either the 'dot' or the 'x'" - so changing things from the keypad perspective.

If you checked and they're all the same thing, disregard. Just a gut reaction more than anything.

github-actions · 2023-09-12T17:26:11Z

Size Change: +95 B (0%)

Total Size: 848 kB

Filename	Size	Change
`packages/math-input/dist/es/index.js`	104 kB	+93 B (0%)
`packages/perseus/dist/es/index.js`	396 kB	+2 B (0%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38 kB
`packages/kmath/dist/es/index.js`	4.12 kB
`packages/perseus-core/dist/es/index.js`	55 B
`packages/perseus-editor/dist/es/index.js`	268 kB
`packages/perseus-error/dist/es/index.js`	705 B
`packages/perseus-linter/dist/es/index.js`	21.2 kB
`packages/pure-markdown/dist/es/index.js`	3.64 kB
`packages/simple-markdown/dist/es/index.js`	12.6 kB

_{compressed-size-action}

SonicScrewdriver · 2023-09-12T17:27:08Z

packages/perseus/src/widgets/expression.tsx

@@ -610,11 +610,13 @@ export class Expression extends React.Component<Props, ExpressionState> {
 */
 const keypadConfigurationForProps = (
    widgetOptions: PerseusExpressionWidgetOptions,
-) => {
+): KeypadConfiguration => {


Added a type as we were missing one.

SonicScrewdriver · 2023-09-12T17:27:24Z

packages/perseus/src/widgets/expression.tsx

@@ -642,20 +644,20 @@ const keypadConfigurationForProps = (

    // TODO(charlie): Alert the keypad as to which of these symbols should be
    // treated as functions.
-    const extraVariables = Object.keys(uniqueExtraVariables);
+    const extraVariables: Key[] = Object.keys(uniqueExtraVariables) as Key[];


Necessary changes to support the new return type.

This seems suspicious to me; if we need to coerce this at all, I think it needs to be ReadonlyArray<Key> rather than Key[]. I imagine the source of the issue is elsewhere though...

Oh this might be part of the problem:

if (!extraKeys.length) { // If there are no extra symbols available, we include Pi anyway, so // that the "extra symbols" button doesn't appear empty. extraKeys.push("PI"); }

We won't be using the long-press "extra symbols" button in v2 keypad (though we still need it while v1 is alive). 🤔 Anyway, I think we could still figure out a way to make it ReadonlyArray<Key>, otherwise I wonder if this is a net loss: we made keypadConfigurationForProps more explicit, but had to add a bunch of Key[] which we typically try to avoid.

SonicScrewdriver · 2023-09-12T19:41:36Z

NOTE : This will not be landed until given the all-clear, as we're currently in a code freeze.

…obile story

handeyeco

My main asks:

Confirm that existing code uses convertDotToTimes, multiplicationDot, and times interchangeably and that they're not subtly different.
Explore options for using ReadonlyArray<Key> vs Key[]

handeyeco · 2023-09-19T14:36:32Z

packages/math-input/src/components/keypad/keypad-mathquill.stories.tsx

@@ -78,7 +78,7 @@ export function V2KeypadWithMathquill() {
                            basicRelations
                            divisionKey
                            logarithms
-                            multiplicationDot
+                            convertDotToTimes


I'm sure you're fine doing this, but I want to confirm that we're not mixing up concepts. In my head:

convertDotToTimes meant "when adding things to the input, convert the symbol 'dot' to be 'x'" - so doing the conversion from the perspective of the input.

multiplicationDot/times meant "in the keypad, show either the 'dot' or the 'x'" - so changing things from the keypad perspective.

If you checked and they're all the same thing, disregard. Just a gut reaction more than anything.

handeyeco · 2023-09-19T14:43:13Z

.changeset/rude-geese-destroy.md

+"@khanacademy/math-input": minor
+"@khanacademy/perseus": minor


The external keypad API is breaking here, so I think math-input would want a major bump.

handeyeco · 2023-09-19T14:47:37Z

packages/math-input/src/components/keypad/keypad-button.tsx

@@ -38,6 +38,7 @@ export const KeypadButton = ({
                gridRow: coord[1] + 1,
                ...style,
            }}
+            testId={keyConfig.id}


I understand the need in this case: the difference between the "dot" and the "x" is visual and they'll likely have the same label. I generally feel opposed to test IDs though - especially for interactive elements - and I worry that having them will encourage others to use them.

I guess if there's no other solution, commenting why we need them for "dot" vs "times" and mentioning that using getByRole is preferable would be nice.

handeyeco · 2023-09-19T14:53:06Z

packages/math-input/src/types.ts

@@ -56,6 +56,7 @@ export type KeyConfig = NonManyKeyConfig | ManyKeyConfig;
 export type KeypadConfiguration = {
    keypadType: KeypadType;
    extraKeys?: ReadonlyArray<Key>;
+    times?: boolean;


If we're trying to consolidate naming, would it be worth making this the same as the others? I know we can't really change what's in the datastore, but by the time we get to KeypadConfiguration I think we could rename it.

handeyeco · 2023-09-19T15:02:20Z

packages/perseus/src/widgets/expression.tsx

@@ -642,20 +644,20 @@ const keypadConfigurationForProps = (

    // TODO(charlie): Alert the keypad as to which of these symbols should be
    // treated as functions.
-    const extraVariables = Object.keys(uniqueExtraVariables);
+    const extraVariables: Key[] = Object.keys(uniqueExtraVariables) as Key[];


This seems suspicious to me; if we need to coerce this at all, I think it needs to be ReadonlyArray<Key> rather than Key[]. I imagine the source of the issue is elsewhere though...

Oh this might be part of the problem:

if (!extraKeys.length) { // If there are no extra symbols available, we include Pi anyway, so // that the "extra symbols" button doesn't appear empty. extraKeys.push("PI"); }

We won't be using the long-press "extra symbols" button in v2 keypad (though we still need it while v1 is alive). 🤔 Anyway, I think we could still figure out a way to make it ReadonlyArray<Key>, otherwise I wonder if this is a net loss: we made keypadConfigurationForProps more explicit, but had to add a bunch of Key[] which we typically try to avoid.

handeyeco · 2023-09-19T15:03:54Z

packages/perseus/src/widgets/expression.tsx

    if (!extraKeys.length) {
        // If there are no extra symbols available, we include Pi anyway, so
        // that the "extra symbols" button doesn't appear empty.
        extraKeys.push("PI");
    }

-    return {keypadType, extraKeys};
+    return {keypadType, extraKeys, times};


Super nit

Suggested change

return {keypadType, extraKeys, times};

return {

keypadType,

extraKeys,

times: widgetOptions.times

};

handeyeco · 2023-09-19T15:05:24Z

packages/math-input/src/index.ts

@@ -41,5 +41,8 @@ export type {default as Keys} from "./data/keys";
 export {default as KeyConfigs} from "./data/key-configs";
 export {type KeyType, KeypadType} from "./enums";

+// Keypad Configuration type
+export {type KeypadConfiguration} from "./types";


We already have export type {KeypadAPI} from "./types";, could we add KeypadConfiguration to that?

handeyeco · 2023-09-19T15:07:52Z

packages/math-input/src/full-math-input.stories.tsx

    // Whether to use v1 or v2 keypad
    const [v2Keypad, setV2Keypad] = React.useState<boolean>(true);
    // Whether the keypad is open or not
    const [keypadOpen, setKeypadOpen] = React.useState<boolean>(false);

+    const input = React.useRef<any>(null);


Would something like this work?

Suggested change

const input = React.useRef<any>(null);

const input = React.useRef<typeof KeypadInput>(null);

…d expression story is providing the V2keypad.

…ocally for me.

coveralls · 2023-09-20T15:32:29Z

coverage: 69.337% (-0.007%) from 69.344% when pulling babda8b on LC-1092 into 91f88d0 on main.

SonicScrewdriver self-assigned this Sep 12, 2023

khan-actions-bot requested a review from a team September 12, 2023 17:22

SonicScrewdriver commented Sep 12, 2023

View reviewed changes

SonicScrewdriver added 3 commits September 13, 2023 10:26

Added logic to convertDotToTimes for the v2 keypad.

5ad9744

Undid changes to expression widget as I moved the tests to the full m…

bac75af

…obile story

Fixing a change to the story during rebase.

dc407de

SonicScrewdriver force-pushed the LC-1092 branch from efbff02 to dc407de Compare September 13, 2023 17:28

SonicScrewdriver added 2 commits September 13, 2023 13:16

Updated snapshots and added a fix for typecheck

3695cf7

Added a test to verify changes + changeset.

15e4e9d

SonicScrewdriver requested a review from handeyeco September 14, 2023 15:27

handeyeco approved these changes Sep 19, 2023

View reviewed changes

SonicScrewdriver and others added 7 commits September 19, 2023 09:18

Updated types and added a comment regarding the use of testId. Ensure…

2a670ae

…d expression story is providing the V2keypad.

New changeset.

7e6bea2

Minor nit from PR.

369bc1c

Fixed some types and fixed the recursion in the story.

ad04519

Added a comment to try to re-prompt github as the tests are passing l…

bc6d3ff

…ocally for me.

Merge branch 'main' into LC-1092

740e703

update snapshots

babda8b

SonicScrewdriver merged commit 609aeb0 into main Sep 20, 2023
12 checks passed

SonicScrewdriver deleted the LC-1092 branch September 20, 2023 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added logic to convertDotToTimes for the v2 keypad. #722

Added logic to convertDotToTimes for the v2 keypad. #722

SonicScrewdriver commented Sep 12, 2023 •

edited

Loading

changeset-bot bot commented Sep 12, 2023 •

edited

Loading

khan-actions-bot commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading

SonicScrewdriver Sep 12, 2023 •

edited

Loading

handeyeco Sep 19, 2023

github-actions bot commented Sep 12, 2023 •

edited

Loading

SonicScrewdriver Sep 12, 2023

SonicScrewdriver Sep 12, 2023

handeyeco Sep 19, 2023

SonicScrewdriver commented Sep 12, 2023

handeyeco left a comment

handeyeco Sep 19, 2023

handeyeco Sep 19, 2023

handeyeco Sep 19, 2023

handeyeco Sep 19, 2023

handeyeco Sep 19, 2023

handeyeco Sep 19, 2023

handeyeco Sep 19, 2023

handeyeco Sep 19, 2023

coveralls commented Sep 20, 2023

		"@khanacademy/math-input": minor
		"@khanacademy/perseus": minor

	const input = React.useRef<any>(null);
	const input = React.useRef<typeof KeypadInput>(null);

Added logic to convertDotToTimes for the v2 keypad. #722

Added logic to convertDotToTimes for the v2 keypad. #722

Conversation

SonicScrewdriver commented Sep 12, 2023 • edited Loading

Summary:

Test plan:

changeset-bot bot commented Sep 12, 2023 • edited Loading

🦋 Changeset detected

khan-actions-bot commented Sep 12, 2023 • edited Loading

Gerald

github-actions bot commented Sep 12, 2023 • edited Loading

npm Snapshot: Published

SonicScrewdriver Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonicScrewdriver commented Sep 12, 2023

handeyeco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 20, 2023

SonicScrewdriver commented Sep 12, 2023 •

edited

Loading

changeset-bot bot commented Sep 12, 2023 •

edited

Loading

khan-actions-bot commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading

SonicScrewdriver Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading