-
Notifications
You must be signed in to change notification settings - Fork 351
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?
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
Size Change: -611 B (-0.05%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
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.
Most of this is great. 🎉
Requesting changes for the comment about default props not being necessary for the SortableItem
changes.
getInitialState() { | ||
return {dragHover: false}; | ||
}, | ||
handleDrop: function (e) { | ||
} |
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.
When initial state doesn't depend on anything, I typically avoid the function getInitialState()
and just define state like this:
state: State = {dragHover: false};
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (658efd0) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1893 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1893 |
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.
Thanks for doing this upgrade! I left a few questions and suggestions inline. In particular, I'd like to confirm that it's safe to use this.props.components
instead of this.state.components
in sortable.tsx
before this lands.
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.
LGTM!
Summary:
Removing createReactClass usage in the following three files:
Issue: LEMS-365
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.