-
Notifications
You must be signed in to change notification settings - Fork 354
[DRAFT]: Remove unused code and usages of findDOMNode #2769
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
462b977
0531062
504bb38
10b363c
ab60dc1
79b40f8
e9a8587
8b935dc
9e27541
4a96131
83156df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@khanacademy/perseus": minor | ||
--- | ||
|
||
Remove unused code and usages of findDOMNode |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,6 @@ import {linterContextDefault} from "@khanacademy/perseus-linter"; | |
import {StyleSheet} from "aphrodite"; | ||
import classNames from "classnames"; | ||
import * as React from "react"; | ||
import ReactDOM from "react-dom"; | ||
import _ from "underscore"; | ||
|
||
import {PerseusI18nContext} from "../../components/i18n-context"; | ||
|
@@ -115,6 +114,7 @@ type State = { | |
class Matrix extends React.Component<Props, State> implements Widget { | ||
static contextType = PerseusI18nContext; | ||
declare context: React.ContextType<typeof PerseusI18nContext>; | ||
answerRefs: Record<string, HTMLInputElement> = {}; | ||
|
||
// @ts-expect-error - TS2564 - Property 'cursorPosition' has no initializer and is not definitely assigned in the constructor. | ||
cursorPosition: [number, number]; | ||
|
@@ -170,9 +170,8 @@ class Matrix extends React.Component<Props, State> implements Widget { | |
|
||
focusInputPath: (arg1: any) => void = (path) => { | ||
const inputID = getRefForPath(path); | ||
// eslint-disable-next-line react/no-string-refs | ||
// @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. | ||
this.refs[inputID].focus(); | ||
const inputComponent = this.answerRefs[inputID]; | ||
inputComponent.focus(); | ||
}; | ||
|
||
blurInputPath: (arg1: any) => void = (path) => { | ||
|
@@ -181,15 +180,14 @@ class Matrix extends React.Component<Props, State> implements Widget { | |
} | ||
|
||
const inputID = getRefForPath(path); | ||
// eslint-disable-next-line react/no-string-refs | ||
// @ts-expect-error - TS2339 - Property 'blur' does not exist on type 'ReactInstance'. | ||
this.refs[inputID].blur(); | ||
const inputComponent = this.answerRefs[inputID]; | ||
inputComponent.blur(); | ||
}; | ||
|
||
getDOMNodeForPath(path: FocusPath) { | ||
const inputID = getRefForPath(path); | ||
// eslint-disable-next-line react/no-string-refs | ||
return ReactDOM.findDOMNode(this.refs[inputID]); | ||
const inputRef = this.answerRefs[inputID]; | ||
return inputRef; | ||
} | ||
|
||
handleKeyDown: (arg1: any, arg2: any, arg3: any) => void = ( | ||
|
@@ -201,8 +199,8 @@ class Matrix extends React.Component<Props, State> implements Widget { | |
const maxCol = this.props.matrixBoardSize[1]; | ||
let enterTheMatrix = null; | ||
|
||
// eslint-disable-next-line react/no-string-refs | ||
const curInput = this.refs[getRefForPath(getInputPath(row, col))]; | ||
const inputID = getRefForPath(getInputPath(row, col)); | ||
const curInput = this.answerRefs[inputID]; | ||
// @ts-expect-error - TS2339 - Property 'getStringValue' does not exist on type 'ReactInstance'. | ||
const curValueString = curInput.getStringValue(); | ||
// @ts-expect-error - TS2339 - Property 'getSelectionStart' does not exist on type 'ReactInstance'. | ||
|
@@ -243,22 +241,19 @@ class Matrix extends React.Component<Props, State> implements Widget { | |
e.preventDefault(); | ||
|
||
// Focus the input and move the cursor to the end of it. | ||
// eslint-disable-next-line react/no-string-refs | ||
const input = this.refs[getRefForPath(nextPath)]; | ||
const inputID = getRefForPath(nextPath); | ||
const input = this.answerRefs[inputID]; | ||
|
||
// Multiply by 2 to ensure the cursor always ends up at the end; | ||
// Opera sometimes sees a carriage return as 2 characters. | ||
// @ts-expect-error - TS2339 - Property 'getStringValue' does not exist on type 'ReactInstance'. | ||
const inputValString = input.getStringValue(); | ||
const valueLength = inputValString.length * 2; | ||
|
||
// @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. | ||
input.focus(); | ||
if (e.key === "ArrowRight") { | ||
// @ts-expect-error - TS2339 - Property 'setSelectionRange' does not exist on type 'ReactInstance'. | ||
input.setSelectionRange(0, 0); | ||
} else { | ||
// @ts-expect-error - TS2339 - Property 'setSelectionRange' does not exist on type 'ReactInstance'. | ||
input.setSelectionRange(valueLength, valueLength); | ||
} | ||
} | ||
|
@@ -377,9 +372,13 @@ class Matrix extends React.Component<Props, State> implements Widget { | |
className: outside | ||
? "outside" | ||
: "inside", | ||
ref: getRefForPath( | ||
getInputPath(row, col), | ||
), | ||
ref: (ref) => { | ||
this.answerRefs[ | ||
getRefForPath( | ||
getInputPath(row, col), | ||
) | ||
] = ref; | ||
}, | ||
// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | ||
value: rowVals ? rowVals[col] : null, | ||
style: { | ||
|
@@ -462,7 +461,6 @@ class Matrix extends React.Component<Props, State> implements Widget { | |
<SimpleKeypadInput | ||
{...inputProps} | ||
style={style} | ||
scrollable={true} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a nonsense property that doesn't exist, so I've removed it! We simply didn't catch it before as SimpleKeypadInput wasn't very specific about props |
||
keypadElement={ | ||
this.props.keypadElement | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import {number as knumber, KhanMath} from "@khanacademy/kmath"; | ||
import * as React from "react"; | ||
import ReactDOM from "react-dom"; | ||
import _ from "underscore"; | ||
|
||
import Graphie from "../../components/graphie"; | ||
|
@@ -247,6 +246,8 @@ class NumberLine extends React.Component<Props, State> implements Widget { | |
static contextType = PerseusI18nContext; | ||
declare context: React.ContextType<typeof PerseusI18nContext>; | ||
|
||
tickControlRef: HTMLInputElement | null = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's only ever one input in NumberLine, so I figured I'd simplify this accordingly. |
||
|
||
static defaultProps: DefaultProps = { | ||
range: [0, 10], | ||
labelStyle: "decimal", | ||
|
@@ -346,41 +347,39 @@ class NumberLine extends React.Component<Props, State> implements Widget { | |
|
||
focus() { | ||
if (this.props.isTickCtrl) { | ||
// eslint-disable-next-line react/no-string-refs | ||
// @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. | ||
this.refs["tick-ctrl"].focus(); | ||
this.tickControlRef?.focus(); | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
focusInputPath: (arg1: any) => void = (path) => { | ||
if (path.length === 1) { | ||
// eslint-disable-next-line react/no-string-refs | ||
// @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. | ||
this.refs[path[0]].focus(); | ||
this.tickControlRef?.focus(); | ||
} | ||
}; | ||
|
||
blurInputPath: (arg1: any) => void = (path) => { | ||
if (path.length === 1) { | ||
// eslint-disable-next-line react/no-string-refs | ||
// @ts-expect-error - TS2339 - Property 'blur' does not exist on type 'ReactInstance'. | ||
this.refs[path[0]].blur(); | ||
this.tickControlRef?.blur(); | ||
} | ||
}; | ||
|
||
// There's only one input path for the tick control, but the renderer | ||
// expects this method to be implemented. | ||
getInputPaths: () => ReadonlyArray<ReadonlyArray<string>> = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since these methods are part of our our main renderer logic, I didn't want to mess with them too much. I think improving this will require a more holistic and modern approach to our focus handling across Perseus (perhaps a context?). |
||
if (this.props.isTickCtrl) { | ||
return [["tick-ctrl"]]; | ||
} | ||
return []; | ||
}; | ||
|
||
// This consumes the input path returned by getInputPaths, | ||
// and returns the DOM node for the tick control input. | ||
getDOMNodeForPath(inputPath: FocusPath) { | ||
// If we have a tick control, return the DOM node for the tick control input. | ||
if (inputPath?.length === 1) { | ||
// eslint-disable-next-line react/no-string-refs | ||
return ReactDOM.findDOMNode(this.refs[inputPath[0]]); | ||
return this.tickControlRef; | ||
} | ||
return null; | ||
} | ||
|
@@ -679,8 +678,9 @@ class NumberLine extends React.Component<Props, State> implements Widget { | |
<label> | ||
{strings.numDivisions}{" "} | ||
<Input | ||
// eslint-disable-next-line react/no-string-refs | ||
ref="tick-ctrl" | ||
ref={(ref) => { | ||
this.tickControlRef = ref; | ||
}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooo I can simplify this |
||
value={ | ||
this.state.numDivisionsEmpty | ||
? null | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remove these ts-expect errors as the SimpleKeypadInput never supported these methods, although our TextInput does. (getStringValue, getSelectionStart, getSelectionEnd).
This means that none of this keyboard logic was ever working for our mobile users. However, they were also very unlikely to hit this as it requires hitting specific keys on their mobile device's keyboard.