Skip to content
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

Removing createReactClass usage in 3 files. #1893

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 46 additions & 33 deletions packages/perseus-editor/src/components/drag-target.tsx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -22,53 +22,66 @@
// * custom styles for global drag and dragOver
// * only respond to certain types of drags (only images for instance)!

import createReactClass from "create-react-class";
import PropTypes from "prop-types";
import * as React from "react";

const DragTarget = createReactClass({
propTypes: {
// All props not listed here are forwarded to the root element without
// modification.
onDrop: PropTypes.func.isRequired,
component: PropTypes.any, // component type
shouldDragHighlight: PropTypes.func,
style: PropTypes.any,
},
getDefaultProps: function () {
return {
component: "div",
shouldDragHighlight: () => true,
};
},
getInitialState: function () {
type Props = {
onDrop: (any) => boolean;
component: any;
shouldDragHighlight: (any) => boolean;
style: any;
};

type DefaultProps = {
onDrop: Props["onDrop"];
component: Props["component"];
shouldDragHighlight: Props["shouldDragHighlight"];
style: Props["style"];
};

export class DragTarget extends React.Component<Props> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {

dragHover: any;

static defaultProps: DefaultProps = {
onDrop: () => true,
component: "div",
shouldDragHighlight: () => true,
style: {},
};

getInitialState() {
return {dragHover: false};
},
handleDrop: function (e) {
}
Comment on lines +51 to +53
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

    state: State = {dragHover: false};


handleDrop(e) {
e.stopPropagation();
e.preventDefault();
this.setState({dragHover: false});
this.props.onDrop(e);
},
handleDragEnd: function () {
}

handleDragEnd() {
this.setState({dragHover: false});
},
handleDragOver: function (e) {
}

handleDragOver(e) {
e.preventDefault();
},
handleDragLeave: function () {
}

handleDragLeave() {
this.setState({dragHover: false});
},
handleDragEnter: function (e) {
}

handleDragEnter(e) {
this.setState({dragHover: this.props.shouldDragHighlight(e)});
},
render: function () {
const opacity = this.state.dragHover ? {opacity: 0.3} : {};
}

render() {
const opacity = this.dragHover ? {opacity: 0.3} : {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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} : {};

const Component = this.props.component;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const forwardProps = Object.assign({}, this.props);
delete forwardProps.component;

Check failure on line 83 in packages/perseus-editor/src/components/drag-target.tsx

View workflow job for this annotation

GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)

The operand of a 'delete' operator cannot be a read-only property.

Check failure on line 83 in packages/perseus-editor/src/components/drag-target.tsx

View workflow job for this annotation

GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)

The operand of a 'delete' operator cannot be a read-only property.

Check failure on line 83 in packages/perseus-editor/src/components/drag-target.tsx

View workflow job for this annotation

GitHub Actions / Publish npm snapshot (ubuntu-latest, 20.x)

The operand of a 'delete' operator cannot be a read-only property.
delete forwardProps.shouldDragHighlight;

Check failure on line 84 in packages/perseus-editor/src/components/drag-target.tsx

View workflow job for this annotation

GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)

The operand of a 'delete' operator cannot be a read-only property.

Check failure on line 84 in packages/perseus-editor/src/components/drag-target.tsx

View workflow job for this annotation

GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)

The operand of a 'delete' operator cannot be a read-only property.

Check failure on line 84 in packages/perseus-editor/src/components/drag-target.tsx

View workflow job for this annotation

GitHub Actions / Publish npm snapshot (ubuntu-latest, 20.x)

The operand of a 'delete' operator cannot be a read-only property.

return (
<Component
Expand All @@ -81,7 +94,7 @@
onDragLeave={this.handleDragLeave}
/>
);
},
});
}
}

export default DragTarget;
171 changes: 107 additions & 64 deletions packages/perseus-editor/src/components/sortable.tsx
Original file line number Diff line number Diff line change
@@ -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;

Check failure on line 6 in packages/perseus-editor/src/components/sortable.tsx

View workflow job for this annotation

GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)

'PT' is assigned a value but never used. Allowed unused vars must match /^_*$/u
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Need to update these values.
type Props = {
className: string;
components: any[];
onReorder: (i: any[]) => void;
style: any;
verify: (i: any) => boolean;
};

type DefaultProps = {
className: Props["className"];
components: Props["components"];
onReorder: Props["onReorder"];
style: Props["style"];
verify: Props["verify"];
};

/**
* Takes an array of components to sort.
* As of 08/05/24, there are two sortable components
* (one in perseus and one in perseus-editor).
* As far as I can tell, this one is only used in ExpressionEditor.
*/
// eslint-disable-next-line react/no-unsafe
const SortableArea = createReactClass({
propTypes: {
className: PT.string,
components: PT.arrayOf(PT.node).isRequired,
onReorder: PT.func.isRequired,
style: PT.any,
verify: PT.func,
},
getDefaultProps: function () {
return {verify: () => true};
},
getInitialState: function () {
export class SortableArea extends React.Component<Props> {
dragging: any;
_dragItems: any;

static defaultProps: DefaultProps = {
className: "",
components: [],
onReorder: () => true,
style: {},
verify: () => true,
};

getInitialState() {
return {
// index of the component being dragged
dragging: null,
components: this.props.components,
};
},
}
// Firefox refuses to drag an element unless you set data on it. Hackily
// add data each time an item is dragged.
componentDidMount: function () {
componentDidMount() {
this._setDragEvents();
},
}

// eslint-disable-next-line react/no-unsafe
UNSAFE_componentWillReceiveProps: function (nextProps) {
UNSAFE_componentWillReceiveProps(nextProps) {
this.setState({components: nextProps.components});
},
componentDidUpdate: function () {
}

componentDidUpdate() {
this._setDragEvents();
},
}

// Alternatively send each handler to each component individually,
// partially applied
onDragStart: function (startIndex) {
onDragStart(startIndex) {
this.setState({dragging: startIndex});
},
onDrop: function () {
}

onDrop() {
// tell the parent component
this.setState({dragging: null});
this.props.onReorder(this.state.components);
},
onDragEnter: function (enterIndex) {
this.props.onReorder(this.props.components);
}

onDragEnter(enterIndex) {
// When a label is first dragged it triggers a dragEnter with itself,
// which we don't care about.
if (this.state.dragging === enterIndex) {
if (this.dragging === enterIndex) {
return;
}

const newComponents = this.state.components.slice();
const newComponents = this.props.components.slice();

// splice the tab out of its old position
const removed = newComponents.splice(this.state.dragging, 1);
const removed = newComponents.splice(this.dragging, 1);
// ... and into its new position
newComponents.splice(enterIndex, 0, removed[0]);

Expand All @@ -75,15 +97,18 @@
});
}
return verified;
},
_listenEvent: function (e) {
}

_listenEvent(e) {
e.dataTransfer.setData("hackhackhack", "because browsers!");
},
_cancelEvent: function (e) {
}

_cancelEvent(e) {
// prevent the browser from redirecting to 'because browsers!'
e.preventDefault();
},
_setDragEvents: function () {
}

_setDragEvents() {
this._dragItems = this._dragItems || [];
const items =
// @ts-expect-error - TS2531 - Object is possibly 'null'
Expand Down Expand Up @@ -117,57 +142,75 @@
dragItem.removeEventListener("dragstart", this._listenEvent);
dragItem.removeEventListener("drop", this._cancelEvent);
}
},
render: function () {
const sortables = this.state.components.map((component, index) => (
}

render() {
const sortables = this.props.components.map((component, index) => (
<SortableItem
index={index}
component={component}
area={this}
key={component.key}
draggable={component.props.draggable}
dragging={index === this.state.dragging}
dragging={index === this.dragging}
/>
));
return (
<ol className={this.props.className} style={this.props.style}>
{sortables}
</ol>
);
},
});
}
}

type ItemProps = {
area: any;
component: any;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dragging: boolean;
draggable: boolean;
index: number;
};

type DefaultItemProps = {
area: ItemProps["area"];
component: ItemProps["component"];
dragging: ItemProps["dragging"];
draggable: ItemProps["draggable"];
index: ItemProps["index"];
};

// An individual sortable item
const SortableItem = createReactClass({
propTypes: {
area: PT.shape({
onDragEnter: PT.func.isRequired,
onDragStart: PT.func.isRequired,
onDrop: PT.func.isRequired,
}),
component: PT.node.isRequired,
dragging: PT.bool.isRequired,
draggable: PT.bool.isRequired,
index: PT.number.isRequired,
},
handleDragStart: function (e) {
export class SortableItem extends React.Component<ItemProps> {
static defaultProps: DefaultItemProps = {
area: "",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

component: {},
dragging: false,
draggable: false,
index: 0,
};

handleDragStart(e) {
e.nativeEvent.dataTransfer.effectAllowed = "move";
this.props.area.onDragStart(this.props.index);
},
handleDrop: function () {
}

handleDrop() {
this.props.area.onDrop(this.props.index);
},
handleDragEnter: function (e) {
}

handleDragEnter(e) {
const verified = this.props.area.onDragEnter(this.props.index);
// Ideally this would change the cursor based on whether this is a
// valid place to drop.
e.nativeEvent.dataTransfer.effectAllowed = verified ? "move" : "none";
},
handleDragOver: function (e) {
}

handleDragOver(e) {
// allow a drop by preventing default handling
e.preventDefault();
},
render: function () {
}

render() {
// I think these might be getting styles from Webapp
let dragState = "sortable-disabled";
if (this.props.dragging) {
Expand All @@ -188,8 +231,8 @@
{this.props.component}
</li>
);
},
});
}
}

const styles = StyleSheet.create({
sortableListItem: {
Expand Down
Loading
Loading