-
Notifications
You must be signed in to change notification settings - Fork 352
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
base: main
Are you sure you want to change the base?
Changes from all commits
4199980
0d3c466
021340c
0987c84
28517ff
2576372
c0947ec
268988b
c740d0b
bc4e6bd
f494bc2
1d3a1de
5acba42
1772774
a23968f
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-editor": patch | ||
--- | ||
|
||
Removing usage of createReactClass from several component files. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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> { | ||||||
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 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. 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. We use
Suggested change
|
||||||
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
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. When initial state doesn't depend on anything, I typically avoid the function 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} : {}; | ||||||
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 should be
Suggested change
|
||||||
const Component = this.props.component; | ||||||
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. I think this and the next few lines can be accomplished in ES6 more idiomatically.
Suggested change
You won't need to |
||||||
|
||||||
const forwardProps = Object.assign({}, this.props); | ||||||
delete forwardProps.component; | ||||||
Check failure on line 83 in packages/perseus-editor/src/components/drag-target.tsx GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)
Check failure on line 83 in packages/perseus-editor/src/components/drag-target.tsx GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)
|
||||||
delete forwardProps.shouldDragHighlight; | ||||||
Check failure on line 84 in packages/perseus-editor/src/components/drag-target.tsx GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)
Check failure on line 84 in packages/perseus-editor/src/components/drag-target.tsx GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x)
|
||||||
|
||||||
return ( | ||||||
<Component | ||||||
|
@@ -81,7 +94,7 @@ | |||||
onDragLeave={this.handleDragLeave} | ||||||
/> | ||||||
); | ||||||
}, | ||||||
}); | ||||||
} | ||||||
} | ||||||
|
||||||
export default DragTarget; |
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; | ||
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. You can delete this and the |
||
|
||
// 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]); | ||
|
||
|
@@ -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' | ||
|
@@ -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; | ||
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. Can we type this as |
||
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: "", | ||
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. I think it'd be better to not provide all these defaults. The |
||
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) { | ||
|
@@ -188,8 +231,8 @@ | |
{this.props.component} | ||
</li> | ||
); | ||
}, | ||
}); | ||
} | ||
} | ||
|
||
const styles = StyleSheet.create({ | ||
sortableListItem: { | ||
|
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.
Are you sure
DragTarget
isn't dead code? I only see one usage, inperseus-editor/src/editor.tsx
. That usage doesn't pass thecomponent
prop, so it seems like thatDragTarget
would throw if it were ever rendered.