Skip to content

Commit

Permalink
Enable "react/recommended" lint ruleset and fix/suppress violations (#…
Browse files Browse the repository at this point in the history
…1937)

## Summary:

While running tests I saw warnings that React components rendered in a list were missing `key`s. This lead me down a path of "is there an ESLint rule for this?" Turns out, there's a nice ruleset for the React ESLint plugin that includes a rule for ensuring JSX elements have a `key` when in a list. 

So, this PR enables the `react/recommended` ESLint rules and fixes/suppresses things. 

Issue: "none"

## Test plan:

`yarn lint`

Also, I opened the affected stories to double-check things.

Author: jeremywiebe

Reviewers: jeremywiebe, benchristel, nishasy

Required Reviewers:

Approved By: benchristel

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1937
  • Loading branch information
jeremywiebe authored Dec 3, 2024
1 parent 2d89ef8 commit 3cdabf1
Show file tree
Hide file tree
Showing 29 changed files with 123 additions and 85 deletions.
5 changes: 5 additions & 0 deletions .changeset/calm-chefs-boil.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

TypeScript fixes
5 changes: 5 additions & 0 deletions .changeset/large-melons-deny.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus-editor": patch
---

Ensure links opening to style guide (Google Docs) set `rel="noreferrer"`
4 changes: 4 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function banImportExtension(extension) {
module.exports = {
extends: [
"@khanacademy",
"plugin:react/recommended",
// This config includes rules from storybook to enforce story best
// practices
"plugin:storybook/recommended",
Expand Down Expand Up @@ -308,6 +309,9 @@ module.exports = {
/**
* react
*/
"react/no-string-refs": "off", // on in react/recommended, but we have #legacy-code
"react/no-find-dom-node": "off", // on in react/recommended, but we have #legacy-code
"react/display-name": "off", // on in react/recommended, but doesn't seem that useful to fix
// This rule results in false-positives when using some types of React
// components (such as functional components or hooks). Since
// TypeScript is already checking that components are only using props
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
} from "@khanacademy/perseus";
import {View} from "@khanacademy/wonder-blocks-core";
import {spacing} from "@khanacademy/wonder-blocks-tokens";
import {useState} from "react";
import * as React from "react";

import {articleWithImages} from "../../../perseus/src/__testdata__/article-renderer.testdata";
import {mockStrings} from "../../../perseus/src/strings";
Expand All @@ -18,7 +18,8 @@ import type {Meta, StoryObj} from "@storybook/react";
import "../styles/perseus-editor.less";

const PreviewWrapper = (props) => {
const [previewDevice, setPreviewDevice] = useState<DeviceType>("phone");
const [previewDevice, setPreviewDevice] =
React.useState<DeviceType>("phone");

return (
<View>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {color, spacing} from "@khanacademy/wonder-blocks-tokens";
import * as React from "react";

import DeviceFramer from "../device-framer";

Expand All @@ -24,8 +25,8 @@ const SampleContent = () => {
}}
>
The DeviceFramer controls the size of the content inside the frame.
So there's not much to look at here except how large each device
type's size is.
So there&apos;s not much to look at here except how large each
device type&apos;s size is.
</div>
);
};
Expand Down
5 changes: 3 additions & 2 deletions packages/perseus-editor/src/components/graph-settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -561,8 +561,9 @@ const GraphSettings = createReactClass({
/>
<InfoTip>
<p>
Create an image in graphie, or use the "Add
image" function to create a background.
Create an image in graphie, or use the
&quot;Add image&quot; function to create a
background.
</p>
</InfoTip>
</div>
Expand Down
6 changes: 3 additions & 3 deletions packages/perseus-editor/src/tex-error-view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ class TexErrorView extends React.Component<Props, State> {
</View>
{showErrors && (
<View style={styles.errorExplanation}>
If your math doesn't display correctly, these errors
might help you troubleshoot. Message #content-kitchen
for help.
If your math doesn&apos;t display correctly, these
errors might help you troubleshoot. Message
#content-kitchen for help.
</View>
)}
{showErrors &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ const WithState = () => {
style={{fontStyle: "italic", marginBottom: spacing.small_12}}
>
<b>Note</b> that this editor has a known-issue where it does not
calculate the image dimensions initially if they aren't
provided. It does update the dimensions when you blur the 'Image
url:' field.
calculate the image dimensions initially if they aren&apos;t
provided. It does update the dimensions when you blur the
&apos;Image url:&apos; field.
</LabelSmall>
<ImageEditor
{...state}
Expand Down
5 changes: 3 additions & 2 deletions packages/perseus-editor/src/widgets/cs-program-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,9 @@ class CSProgramEditor extends React.Component<any> {
}}
/>
<InfoTip>
If you show the editor, you should use the "full-width"
alignment to make room for the width of the editor.
If you show the editor, you should use the
&quot;full-width&quot; alignment to make room for the width
of the editor.
</InfoTip>
<br />
<Checkbox
Expand Down
1 change: 1 addition & 0 deletions packages/perseus-editor/src/widgets/definition-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class DefinitionEditor extends React.Component<Props> {
<a
href="https://docs.google.com/document/d/1udaPef4imOfTMhmLDlWq4SM0mxL0r3YHFZE-5J1uGfo"
target="_blank"
rel="noreferrer"
>
Definition style guide
</a>
Expand Down
13 changes: 7 additions & 6 deletions packages/perseus-editor/src/widgets/dropdown-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ class DropdownEditor extends React.Component<Props> {
The drop down is useful for making inequalities in a
custom format. We normally use the symbols {"<"},{" "}
{">"}, ≤, ≥ (in that order) which you can copy into
the choices. When possible, use the "multiple
choice" answer type instead.
the choices. When possible, use the &quot;multiple
choice&quot; answer type instead.
</p>
</InfoTip>
</div>
Expand All @@ -145,18 +145,19 @@ class DropdownEditor extends React.Component<Props> {
</LabelMedium>
<InfoTip>
<p>
Label text that's read by screen readers. Highly
recommend adding a label here to ensure your
Label text that&apos;s read by screen readers.
Highly recommend adding a label here to ensure your
exercise is accessible. For more information on
writing accessible labels, please see{" "}
<a
href="https://www.w3.org/WAI/tips/designing/#ensure-that-form-elements-include-clearly-associated-labels"
target="_blank"
rel="noreferrer"
>
this article.
</a>{" "}
If left blank, the value will default to "Select an
answer".
If left blank, the value will default to
&quot;Select an answer&quot;.
</p>
</InfoTip>
</div>
Expand Down
27 changes: 15 additions & 12 deletions packages/perseus-editor/src/widgets/expression-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ class ExpressionEditor extends React.Component<Props, State> {

return (
<AnswerOption
key={ans.key}
draggable={true}
considered={ans.considered}
expressionProps={expressionProps}
Expand Down Expand Up @@ -449,13 +450,14 @@ class ExpressionEditor extends React.Component<Props, State> {
/>
<InfoTip>
<p>
Label text that's read by screen readers. Highly
recommend adding a label here to ensure your
Label text that&apos;s read by screen readers.
Highly recommend adding a label here to ensure your
exercise is accessible. For more information on
writting accessible labels, please see{" "}
<a
href="https://www.w3.org/WAI/tips/designing/#ensure-that-form-elements-include-clearly-associated-labels"
target="_blank"
rel="noreferrer"
>
this article.
</a>
Expand All @@ -473,7 +475,8 @@ class ExpressionEditor extends React.Component<Props, State> {
<p>
Single-letter variables listed here will be
interpreted as functions. This let us know that f(x)
means "f of x" and not "f times x".
means &quot;f of x&quot; and not &quot;f times
x&quot;.
</p>
</InfoTip>
</div>
Expand Down Expand Up @@ -588,7 +591,7 @@ class AnswerOption extends React.Component<
onClick={this.handleImSure}
color="destructive"
>
I'm sure!
I&apos;m sure!
</Button>
<Strut size={spacing.small_12} />
<Button size="small" onClick={this.handleCancelDelete} light>
Expand Down Expand Up @@ -639,9 +642,9 @@ class AnswerOption extends React.Component<
/>
<InfoTip>
<p>
The student's answer must be in the same form.
Commutativity and excess negative signs are
ignored.
The student&apos;s answer must be in the same
form. Commutativity and excess negative signs
are ignored.
</p>
</InfoTip>
</div>
Expand All @@ -654,11 +657,11 @@ class AnswerOption extends React.Component<
/>
<InfoTip>
<p>
The student's answer must be fully expanded and
simplified. Answering this equation (x^2+2x+1)
with this factored equation (x+1)^2 will render
this response "Your answer is not fully expanded
and simplified."
The student&apos;s answer must be fully expanded
and simplified. Answering this equation
(x^2+2x+1) with this factored equation (x+1)^2
will render this response &quot;Your answer is
not fully expanded and simplified.&quot;
</p>
</InfoTip>
</div>
Expand Down
32 changes: 16 additions & 16 deletions packages/perseus-editor/src/widgets/input-number-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -168,20 +168,20 @@ class InputNumberEditor extends React.Component<Props> {
</label>
<InfoTip>
<p>
Normally select "will not be graded". This will give
the user a message saying the answer is correct but
not simplified. The user will then have to simplify
it and re-enter, but will not be penalized. (5th
grade and anything after)
Normally select &quot;will not be graded&quot;. This
will give the user a message saying the answer is
correct but not simplified. The user will then have
to simplify it and re-enter, but will not be
penalized. (5th grade and anything after)
</p>
<p>
Select "will be accepted" only if the user is not
expected to know how to simplify fractions yet.
(Anything prior to 5th grade)
Select &quot;will be accepted&quot; only if the user
is not expected to know how to simplify fractions
yet. (Anything prior to 5th grade)
</p>
<p>
Select "will be marked wrong" only if we are
specifically assessing the ability to simplify.
Select &quot;will be marked wrong&quot; only if we
are specifically assessing the ability to simplify.
</p>
</InfoTip>
</div>
Expand Down Expand Up @@ -240,9 +240,9 @@ class InputNumberEditor extends React.Component<Props> {
</select>
<InfoTip>
<p>
Use the default "Numbers" unless the answer must be
in a specific form (e.g., question is about
converting decimals to fractions).
Use the default &quot;Numbers&quot; unless the
answer must be in a specific form (e.g., question is
about converting decimals to fractions).
</p>
</InfoTip>
</div>
Expand All @@ -263,9 +263,9 @@ class InputNumberEditor extends React.Component<Props> {
</label>
<InfoTip>
<p>
Use size "Normal" for all text boxes, unless there
are multiple text boxes in one line and the answer
area is too narrow to fit them.
Use size &quot;Normal&quot; for all text boxes,
unless there are multiple text boxes in one line and
the answer area is too narrow to fit them.
</p>
</InfoTip>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -582,8 +582,9 @@ class InteractiveGraphSettings extends React.Component<Props, State> {
/>
<InfoTip>
<p>
Create an image in graphie, or use the "Add
image" function to create a background.
Create an image in graphie, or use the
&quot;Add image&quot; function to create a
background.
</p>
</InfoTip>
</LabeledRow>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ const LockedEllipseSettings = (props: Props) => {
{labels?.map((label, labelIndex) => (
<LockedLabelSettings
{...label}
key={labelIndex}
expanded={true}
onChangeProps={(newLabel) => {
handleLabelChange(newLabel, labelIndex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ export default function LockedLabelSettings(props: Props) {
<br />
<br />
It is important to use TeX when appropriate for
accessibility. The above example would be read as "This
circle has radius one-half units" by screen readers.
accessibility. The above example would be read as &quot;This
circle has radius one-half units&quot; by screen readers.
</InfoTip>
</View>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ const LockedLineSettings = (props: Props) => {
{labels?.map((label, labelIndex) => (
<LockedLabelSettings
{...label}
key={labelIndex}
expanded={true}
onChangeProps={(newLabel) => {
handleLabelChange(newLabel, labelIndex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ const LockedPointSettings = (props: Props) => {
{labels?.map((label, labelIndex) => (
<LockedLabelSettings
{...label}
key={labelIndex}
containerStyle={
!isDefiningPoint &&
styles.lockedPointLabelContainer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ const LockedPolygonSettings = (props: Props) => {
{labels?.map((label, labelIndex) => (
<LockedLabelSettings
{...label}
key={labelIndex}
expanded={true}
onChangeProps={(newLabel) => {
handleLabelChange(newLabel, labelIndex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ const LockedVectorSettings = (props: Props) => {
{labels?.map((label, labelIndex) => (
<LockedLabelSettings
{...label}
key={labelIndex}
expanded={true}
onChangeProps={(newLabel) => {
handleLabelChange(newLabel, labelIndex);
Expand Down
4 changes: 2 additions & 2 deletions packages/perseus-editor/src/widgets/measurer-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ class MeasurerEditor extends React.Component<Props> {
/>
<InfoTip>
<p>
Create an image in graphie, or use the "Add image"
function to create a background.
Create an image in graphie, or use the &quot;Add
image&quot; function to create a background.
</p>
</InfoTip>
</div>
Expand Down
Loading

0 comments on commit 3cdabf1

Please sign in to comment.