-
Notifications
You must be signed in to change notification settings - Fork 366
Remove some string refs (the more straightforward ones) #3121
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 all commits
2a4ba05
f702f49
354af1b
de0a783
ef4bfdc
e5c3ad2
52d0abc
e36b0dd
db49452
31d4d7c
21ac217
6d02eba
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,6 @@ | ||
| --- | ||
| "@khanacademy/perseus": patch | ||
| "@khanacademy/perseus-editor": patch | ||
| --- | ||
|
|
||
| Remove some string refs (the more straightforward ones) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,8 +101,6 @@ class MatrixEditor extends React.Component<Props> { | |
| {" "} | ||
| Matrix prefix:{" "} | ||
| <Editor | ||
| // eslint-disable-next-line react/no-string-refs | ||
| ref="prefix" | ||
| apiOptions={this.props.apiOptions} | ||
| content={this.props.prefix} | ||
| widgetEnabled={false} | ||
|
|
@@ -115,8 +113,6 @@ class MatrixEditor extends React.Component<Props> { | |
| {" "} | ||
| Matrix suffix:{" "} | ||
| <Editor | ||
| // eslint-disable-next-line react/no-string-refs | ||
| ref="suffix" | ||
|
Contributor
Author
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. Looks like these weren't being used. |
||
| apiOptions={this.props.apiOptions} | ||
| content={this.props.suffix} | ||
| widgetEnabled={false} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,8 +95,6 @@ class MeasurerEditor extends React.Component<Props> { | |
| <input | ||
| type="text" | ||
| className="perseus-widget-measurer-url" | ||
| // eslint-disable-next-line react/no-string-refs | ||
| ref="image-url" | ||
|
Contributor
Author
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. Looks like this wasn't being used. |
||
| defaultValue={image.url} | ||
| onChange={this._changeUrl} | ||
| /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,8 +37,6 @@ class PassageEditor extends React.Component<Props> { | |
| render(): React.ReactNode { | ||
| const passageEditor = ( | ||
| <Editor | ||
| // eslint-disable-next-line react/no-string-refs | ||
| ref="passage-editor" | ||
| apiOptions={this.props.apiOptions} | ||
| content={this.props.passageText} | ||
| widgetEnabled={false} | ||
|
|
@@ -52,8 +50,6 @@ class PassageEditor extends React.Component<Props> { | |
| ); | ||
| const footnotesEditor = ( | ||
| <Editor | ||
| // eslint-disable-next-line react/no-string-refs | ||
| ref="passage-footnotes-editor" | ||
|
Contributor
Author
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. Looks like these weren't being used. |
||
| apiOptions={this.props.apiOptions} | ||
| content={this.props.footnotes} | ||
| widgetEnabled={false} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,12 +29,6 @@ class TableEditor extends React.Component<Props> { | |
| static defaultProps: TableDefaultWidgetOptions = | ||
| tableLogic.defaultWidgetOptions; | ||
|
|
||
| numberOfColumns = React.createRef<components.NumberInput>(); | ||
|
|
||
| focus: () => void = () => { | ||
| this.numberOfColumns.current?.focus(); | ||
| }; | ||
|
|
||
|
Contributor
Author
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. It looks like this focus method wasn't actually ever being used. And that also means none of the refs were being used. |
||
| onSizeInput: (arg1: number, arg2: number) => void = ( | ||
| numRawRows, | ||
| numRawColumns, | ||
|
|
@@ -117,7 +111,6 @@ class TableEditor extends React.Component<Props> { | |
| <label> | ||
| Number of columns:{" "} | ||
| <NumberInput | ||
| ref={this.numberOfColumns} | ||
| value={this.props.columns} | ||
| onChange={(val) => { | ||
| if (val) { | ||
|
|
@@ -132,8 +125,6 @@ class TableEditor extends React.Component<Props> { | |
| <label> | ||
| Number of rows:{" "} | ||
| <NumberInput | ||
| // eslint-disable-next-line react/no-string-refs | ||
| ref="numberOfRows" | ||
| value={this.props.rows} | ||
| onChange={(val) => { | ||
| if (val) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -524,8 +524,6 @@ class SvgImage extends React.Component<Props, State> { | |
| // documented where it came from. | ||
| graphie = ( | ||
| <Graphie | ||
| // eslint-disable-next-line react/no-string-refs | ||
| ref="graphie" | ||
|
Contributor
Author
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. Looks like this wasn't being used. |
||
| box={box} | ||
| scale={[40 * this.props.scale, 40 * this.props.scale]} | ||
| range={this.state.range} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,9 @@ export class Expression extends React.Component<Props> implements Widget { | |
| _textareaId = `expression_textarea_${Date.now()}`; | ||
| _isMounted = false; | ||
|
|
||
| keypadInputRef = React.createRef<KeypadInput>(); | ||
| mathInputRef = React.createRef<MathInput>(); | ||
|
|
||
| static defaultProps: DefaultProps = { | ||
| times: false, | ||
| functions: [], | ||
|
|
@@ -119,10 +122,12 @@ export class Expression extends React.Component<Props> implements Widget { | |
| // HACK: imperatively add an ID onto the Mathquill input | ||
| // (which in mobile is a span; desktop a textarea) | ||
| // in order to associate a visual label with it | ||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | ||
| if (this.refs.input) { | ||
| const isMobile = this.props.apiOptions.customKeypad; | ||
| const container = ReactDOM.findDOMNode(this.refs.input); | ||
| const isMobile = this.props.apiOptions.customKeypad; | ||
| const inputComponent = isMobile | ||
| ? this.keypadInputRef.current | ||
| : this.mathInputRef.current; | ||
| if (inputComponent) { | ||
| const container = ReactDOM.findDOMNode(inputComponent); | ||
| const selector = isMobile ? ".mq-textarea > span" : "textarea"; | ||
| const inputElement = (container as Element).querySelector(selector); | ||
| inputElement?.setAttribute("id", this._textareaId); | ||
|
|
@@ -166,38 +171,33 @@ export class Expression extends React.Component<Props> implements Widget { | |
|
|
||
| focus: () => boolean = () => { | ||
| if (this.props.apiOptions.customKeypad) { | ||
| // eslint-disable-next-line react/no-string-refs | ||
| // @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. | ||
| this.refs.input.focus(); | ||
| // KeypadInput's focus() requires a setKeypadActive parameter, | ||
| // but we don't have access to it here, so we just don't call focus | ||
| // TODO: Investigate if this is needed | ||
|
Contributor
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. Just want to make sure — did you get a chance to test this on mobile? Since Also, should we create a ticket to track the TODO here so it doesn't get lost? |
||
| } else { | ||
| this.mathInputRef.current?.focus(); | ||
| } | ||
| return true; | ||
| }; | ||
|
|
||
| // TODO(LEMS-2656): remove TS suppression | ||
| // @ts-expect-error: Type 'FocusPath' is not assignable to type 'InputPath'. | ||
| focusInputPath(inputPath: InputPath) { | ||
| // eslint-disable-next-line react/no-string-refs | ||
| // @ts-expect-error - TS2339 - Property 'focus' does not exist on type 'ReactInstance'. | ||
| this.refs.input.focus(); | ||
| this.mathInputRef.current?.focus(); | ||
| } | ||
|
|
||
| // TODO(LEMS-2656): remove TS suppression | ||
| // @ts-expect-error: Type 'FocusPath' is not assignable to type 'InputPath'. | ||
| blurInputPath(inputPath: InputPath) { | ||
| // eslint-disable-next-line react/no-string-refs | ||
| // @ts-expect-error - TS2339 - Property 'blur' does not exist on type 'ReactInstance'. | ||
| if (typeof this.refs.input?.blur === "function") { | ||
| // eslint-disable-next-line react/no-string-refs | ||
| // @ts-expect-error - TS2339 - Property 'blur' does not exist on type 'ReactInstance'. | ||
| this.refs.input?.blur(); | ||
| blurInputPath() { | ||
| if (this.props.apiOptions.customKeypad) { | ||
| this.keypadInputRef.current?.blur(); | ||
| } else { | ||
| this.mathInputRef.current?.blur(); | ||
| } | ||
| } | ||
|
|
||
| // HACK(joel) | ||
| insert(keyPressed: KeypadKey) { | ||
| // eslint-disable-next-line react/no-string-refs | ||
| // @ts-expect-error - TS2339 - Property 'insert' does not exist on type 'ReactInstance'. | ||
| this.refs.input.insert(keyPressed); | ||
| // Only the Perseus MathInput has the insert method | ||
| this.mathInputRef.current?.insert(keyPressed); | ||
| } | ||
|
|
||
| getInputPaths: () => ReadonlyArray<ReadonlyArray<any>> = () => { | ||
|
|
@@ -240,8 +240,7 @@ export class Expression extends React.Component<Props> implements Widget { | |
| </LabelSmall> | ||
| )} | ||
| <KeypadInput | ||
| // eslint-disable-next-line react/no-string-refs | ||
| ref="input" | ||
| ref={this.keypadInputRef} | ||
| ariaLabel={ | ||
| this.props.ariaLabel || | ||
| this.context.strings.mathInputBox | ||
|
|
@@ -278,8 +277,7 @@ export class Expression extends React.Component<Props> implements Widget { | |
| {/* eslint-disable-next-line jsx-a11y/no-static-element-interactions -- TODO(LEMS-2871): Address a11y error */} | ||
| <div className="perseus-widget-expression"> | ||
| <MathInput | ||
| // eslint-disable-next-line react/no-string-refs | ||
| ref="input" | ||
| ref={this.mathInputRef} | ||
| value={this.props.userInput} | ||
| onChange={this.changeAndTrack} | ||
| convertDotToTimes={this.props.times} | ||
|
|
||
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.
It looks like this wasn't being used anywhere.