Removing createReactClass usage in 3 files. #1893

catandthemachines · 2024-11-20T19:39:52Z

Summary:

Removing createReactClass usage in the following three files:

drag-target.tsx
sortable.tsx
stateful-editor-page.tsx

Test plan:

Run "yarn test" and confirm no errors or regressions are introduced.
Run "yarn start" and look and view if there are any issues with any Perseus components.

khan-actions-bot · 2024-11-20T19:40:08Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/khaki-keys-approve.md, packages/perseus-editor/src/editor.tsx, packages/perseus-editor/src/stateful-editor-page.tsx, packages/perseus-editor/src/components/drag-target.tsx, packages/perseus-editor/src/components/sortable.tsx, packages/perseus-editor/src/widgets/expression-editor.tsx

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

github-actions · 2024-11-20T19:44:36Z

Size Change: -197 B (-0.02%)

Total Size: 1.29 MB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	698 kB	-197 B (-0.03%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	77.9 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/index.js`	424 kB
`packages/perseus/dist/es/strings.js`	3.57 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

jeremywiebe

Most of this is great. 🎉

Requesting changes for the comment about default props not being necessary for the SortableItem changes.

jeremywiebe · 2024-11-20T20:55:50Z

packages/perseus-editor/src/components/drag-target.tsx

+    }
+
+    render() {
+        const opacity = this.dragHover ? {opacity: 0.3} : {};
        const Component = this.props.component;


I think this and the next few lines can be accomplished in ES6 more idiomatically.

Suggested change

const Component = this.props.component;

const {component: Component, shouldDragHighlight, ...forwardProps } = this.props.component;

You won't need to delete props off of the object then.

jeremywiebe · 2024-11-20T20:56:21Z

packages/perseus-editor/src/components/drag-target.tsx

+    style: Props["style"];
+};
+
+export class DragTarget extends React.Component<Props> {


This is an example of a component that is so simple it could be converted all the way to a functional component. It consists of rendering a component and some callback handlers. These are all easily modelled in a functional component. :)

Not required for this PR, but if you're feeling adventurous, go for it.

jeremywiebe · 2024-11-20T20:56:38Z

packages/perseus-editor/src/components/sortable.tsx

@@ -1,69 +1,91 @@
 import {css, StyleSheet} from "aphrodite";
-import createReactClass from "create-react-class";
 import PropTypes from "prop-types";
 import * as React from "react";
 import ReactDOM from "react-dom";

 const PT = PropTypes;


You can delete this and the prop-types import now, I think.

jeremywiebe · 2024-11-20T20:57:52Z

packages/perseus-editor/src/components/drag-target.tsx

+    getInitialState() {
        return {dragHover: false};
-    },
-    handleDrop: function (e) {
+    }


When initial state doesn't depend on anything, I typically avoid the function getInitialState() and just define state like this:

state: State = {dragHover: false};

jeremywiebe · 2024-11-20T20:58:21Z

packages/perseus-editor/src/components/drag-target.tsx

+    style: Props["style"];
+};
+
+export class DragTarget extends React.Component<Props> {


We use state in this component, so it'd be a good idea to provide a State generic argument.

Suggested change

export class DragTarget extends React.Component<Props> {

export class DragTarget extends React.Component<Props, State> {

jeremywiebe · 2024-11-20T21:01:16Z

packages/perseus-editor/src/components/sortable.tsx

+
+type ItemProps = {
+    area: any;
+    component: any;


Can we type this as React.ReactNode? As far as I can see there are no constraints on what this is except that it is something React can render.

jeremywiebe · 2024-11-20T21:04:15Z

packages/perseus-editor/src/components/sortable.tsx

-    handleDragStart: function (e) {
+export class SortableItem extends React.Component<ItemProps> {
+    static defaultProps: DefaultItemProps = {
+        area: "",


I think it'd be better to not provide all these defaults. The Sortable that renders these items looks like it provides all of these props. By making defaults for these props, we make the prop optional, and force this component to have a good default. In the case of area, the default is of the wrong type as it should be an object that provides three callback functions: onDragEnter, onDragStart, and onDrop.

jeremywiebe · 2024-11-20T21:07:14Z

packages/perseus-editor/src/stateful-editor-page.tsx

 import * as React from "react";
 import _ from "underscore";

 import EditorPage from "./editor-page";

+type Props = {
+    componentClass: any;


I think you can type this as React.ComponentType.

benchristel · 2024-11-20T22:08:27Z

packages/perseus-editor/src/components/drag-target.tsx

+    }
+
+    render() {
+        const opacity = this.dragHover ? {opacity: 0.3} : {};


This should be this.state.dragHover. Then you don't need the dragHover: any; instance variable above (which is never set).

Suggested change

const opacity = this.dragHover ? {opacity: 0.3} : {};

const opacity = this.state.dragHover ? {opacity: 0.3} : {};

benchristel · 2024-11-20T22:12:34Z

packages/perseus-editor/src/components/drag-target.tsx

Are you sure DragTarget isn't dead code? I only see one usage, in perseus-editor/src/editor.tsx. That usage doesn't pass the component prop, so it seems like that DragTarget would throw if it were ever rendered.

catandthemachines added 13 commits September 16, 2024 12:40

Beginning of removing createReactClass.

4199980

Merge branch 'main' into catjohnson/remove-createreactclass

0d3c466

Merge branch 'main' into catjohnson/remove-createreactclass

021340c

Migrated measure to class structure.

0987c84

Removing createReactClass

28517ff

graph settings updating.

2576372

Merge branch 'main' into catjohnson/remove-createreactclass

c0947ec

Merge branch 'main' into catjohnson/remove-createreactclass

268988b

Merge branch 'main' into catjohnson/remove-createreactclass

c740d0b

Merge branch 'main' into catjohnson/remove-createreactclass

bc4e6bd

undoing measurer changes.

f494bc2

reverting json-editor.tsx

1d3a1de

reverting graph-settings.tsx

5acba42

catandthemachines self-assigned this Nov 20, 2024

khan-actions-bot requested a review from a team November 20, 2024 19:40

catandthemachines requested review from jeremywiebe, benchristel, nishasy, handeyeco and anakaren-rojas November 20, 2024 19:41

jeremywiebe requested changes Nov 20, 2024

View reviewed changes

catandthemachines added 2 commits November 20, 2024 13:41

Add changeset.

1772774

Adding some more typing.

a23968f

benchristel reviewed Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing createReactClass usage in 3 files. #1893

Removing createReactClass usage in 3 files. #1893

catandthemachines commented Nov 20, 2024 •

edited

Loading

khan-actions-bot commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Nov 20, 2024 •

edited

Loading

jeremywiebe left a comment

jeremywiebe Nov 20, 2024

jeremywiebe Nov 20, 2024

jeremywiebe Nov 20, 2024

jeremywiebe Nov 20, 2024

jeremywiebe Nov 20, 2024

jeremywiebe Nov 20, 2024

jeremywiebe Nov 20, 2024

jeremywiebe Nov 20, 2024

benchristel Nov 20, 2024

benchristel Nov 20, 2024

	const Component = this.props.component;
	const {component: Component, shouldDragHighlight, ...forwardProps } = this.props.component;

	export class DragTarget extends React.Component<Props> {
	export class DragTarget extends React.Component<Props, State> {

	const opacity = this.dragHover ? {opacity: 0.3} : {};
	const opacity = this.state.dragHover ? {opacity: 0.3} : {};

Removing createReactClass usage in 3 files. #1893

Are you sure you want to change the base?

Removing createReactClass usage in 3 files. #1893

Conversation

catandthemachines commented Nov 20, 2024 • edited Loading

Summary:

Test plan:

khan-actions-bot commented Nov 20, 2024 • edited Loading

Gerald

github-actions bot commented Nov 20, 2024 • edited Loading

jeremywiebe 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catandthemachines commented Nov 20, 2024 •

edited

Loading

khan-actions-bot commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Nov 20, 2024 •

edited

Loading