-
Notifications
You must be signed in to change notification settings - Fork 25
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
Modernize existing React component code #17
Conversation
renderItem(item) { | ||
}; | ||
|
||
renderItem = (item) => { |
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.
I don't think it's necessary to convert method other than event listener to class properties, this
is still this class because it's not called from event listener.
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.
There are some benefits to use public class fields syntax:
(1). It prevents accidental neglect of binding.
It's actually not necessary to transform all methods to arrow functions (i.e., to bind them), but this behavior is the same as createClass() and we can make sure that we won't accidentally break stuff
(2). It does not have the performance issue.
It avoids the performance issues of approach
Bind in Render
andUse Arrow Function in Render
.
Related:
https://medium.freecodecamp.org/react-binding-patterns-5-approaches-for-handling-this-92c651b5af56
(3). The converting was done by jscodeshift
automatically.
I used react-codemod
, which automatically transforms all methods to arrow functions.
It involves much manual work to convert arrow functions back.
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.
I'm not sure, but it seems to me that the function renderItem
is indeed called from outside. I suspect it is called somewhere in react-bootstrap
library.
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.
- It literally says
It's actually not necessary to transform all methods to arrow functions
- That's right, and we're talking about comparing
bind(this)
and() => listener()
, other function besides event listener does not need bind.
If we're deciding to move back to using normal class method on non event listener, if that's even worth it, we need to make sure it doesn't break anything and this project has no complete tests.
@andrewda gonna need your opinion on this.
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.
Maybe we could leave this as a separate issue? If we decide to move back to normal methods, we need careful reviews. Errors are likely to be missed in this large PR.
If I move some inappropriate methods back to use normal class methods, there would be no diff for that method in this PR, which is almost impossible for reviewers to detect if I make any errors.
|
||
|
||
const FilterDropdownShell = React.createClass({ | ||
class FilterDropdownShell extends React.Component { | ||
render() { |
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.
This can be converted to functional component.
src/components/by-user-view.jsx
Outdated
|
||
|
||
const UsersView = React.createClass({ | ||
class UsersView extends React.Component { | ||
render() { |
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.
This can be converted to functional component.
src/components/colored-icon.jsx
Outdated
@@ -4,7 +4,7 @@ import classnames from 'classnames'; | |||
import {isLight} from '../helpers'; | |||
import {getFilters} from '../route-utils'; | |||
|
|||
const ColoredIcon = React.createClass({ | |||
class ColoredIcon extends React.Component { | |||
render() { |
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.
This can be converted to functional component.
src/components/dashboard.jsx
Outdated
|
||
const RepoItem = React.createClass({ | ||
class RepoItem extends React.Component { | ||
render() { |
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.
This can be converted to functional component.
All component with only render method can be functional component, I'll stop marking.
Build is failing, looks like we need that json generator first. |
Also there's an update on lifecycle method https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#migrating-from-legacy-lifecycles , let's see if we can migrate safely. |
src/components/anonymous-modal.jsx
Outdated
@@ -6,7 +6,7 @@ import CurrentUserStore from '../user-store'; | |||
|
|||
let hasAlreadyShownAnonymousModal = false; | |||
|
|||
const AnonymousModal = React.createClass({ | |||
class AnonymousModal extends React.Component { |
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.
Could just be me, but I much prefer extends Component
over extends React.Component
. (i.e. importing Component
using import React, {Component} from 'react'
)
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.
Yeah, let's do that.
Additionally, babel-plugin-react-require can be used to auto import React when the file only has functional component that doesn't need to extends from Component
I'm willing to merge without updating the lifecycle method in this PR. There's no deprecation warning in 16.3, but the next version will. |
@gitmate-bot rebase |
Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently |
698f26f
to
073b6e2
Compare
Automated rebase with GitMate.io was successful! 🎉 |
src/components/issue.diff
Outdated
@@ -0,0 +1,848 @@ | |||
diff --git a/src/components/issue.jsx b/src/components/issue.jsx |
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.
Unrelated file
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.
Thx. A stupid mistake which should definitely be avoided. 😅
src/components/issue-blurb.jsx
Outdated
@@ -10,7 +10,7 @@ import GithubFlavoredMarkdown from './gfm'; | |||
import Loadable from './loadable'; | |||
import ColoredIcon from './colored-icon'; | |||
|
|||
const IssueOrPullRequestBlurb = React.createClass({ | |||
class IssueOrPullRequestBlurb extends Component { |
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.
More functional component?
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.
Using functional component would lead to an error as it is using this.onClickIcon
here. To be honest, I don't know where the function onClickIcon
comes from. I use grep and find out it's being used in issue-blurb.jsx but defined nowhere.
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.
Ah ok. We'll leave it as it is.
But does it even matter? Since this.onClickIcon
is undefined whether it is using class or functional. It would lead to an error either way.
Probably open an issue after finding where the component is located in the UI, if there's an error.
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.
I just transformed it into the functional component. You are right, the undefined onClickIcon
has nothing to do with whether it is using class or function.
Filed another issue #29 for the undefined onClickIcon
.
src/components/anonymous-modal.jsx
Outdated
function AnonymousModal() { | ||
const onHide = () => { | ||
hasAlreadyShownAnonymousModal = true; | ||
this.setState({ showModal: false}); |
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.
This will introduce error, since it's functional component and has no this.setState
https://jsbin.com/dodalubaxo/edit?html,js,output
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.
And the worst part is that this modal is showed when you visit the issues view
https://deploy-preview-17--coala-gh-board.netlify.com/#/r/coala:coala%7Ccoala-bears%7Cdocumentation%7Ccorobo/kanban
src/components/board.jsx
Outdated
onStop(context) { this.setState({ticks: this.props.progress.ticks, max: this.props.progress.max, message: 'Finished: ' + context}); }, | ||
}; | ||
|
||
onStop = (context) => { this.setState({ticks: this.props.progress.ticks, max: this.props.progress.max, message: 'Finished: ' + context}); }; |
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.
line is longer than github's code review block width.
drop messages: ...
to next line under ticks:
?
or something similar.
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.
Please note that a 'split line' type of change should only be done in this PR where the change already contains a non-whitespace modification to the line.
We dont want git diff -w
for this patch getting extra changes which are line splits.
There are only a few cases in this PR where the very long line is already changing syntax, so we may as well also improve the layout a little.
if (repoInfos.length) { | ||
const [{repoOwner, repoName}] = repoInfos; | ||
|
||
// TODO: Poll all of these so we get updates |
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.
Create a separate PR (to be merged first) remove any of the TODOs which are going to be needlessly moved as part of this PR. #30
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.
Not done yet?
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.
Created a separate PR here: #33
// !isFilteringByColumn && (!getShowEmptyColumns || columnCards.length) | ||
|
||
kanbanColumnCount++; // Count the number of actual columns displayed | ||
/*HACK: Column should handle milestones */ |
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.
this set of comments is duplicated from by-milestone-view.jsx
.
should this handle milestones in by-user-view.jsx
?
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.
Looks like when the author reused the code from by-milestone-view.jsx
, he forgot to delete the unrelated comment.
src/components/etherpad.jsx
Outdated
}, | ||
class EtherpadInner extends Component { | ||
static defaultProps = { | ||
hostName: 'https://openstax-pad.herokuapp.com', |
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.
We shouldnt modernise a component that we are not able to test
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.
React.createClass is not supported in React V16.
A feasible approach is to import createReactClass from "create-react-class";
and substitute React.createClass
with createReactClass
.
); | ||
} | ||
if (issue) { | ||
const isPullRequest = card.isPullRequest() || issue.base; // use .base in case we are given the PR JSON (which does not contain labels) |
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.
line is longer than github review block width
@@ -20,7 +21,7 @@ const AnonymousModal = React.createClass({ | |||
} | |||
|
|||
return ( | |||
<BS.Modal show={showModal} container={this} onHide={onHide}> | |||
<BS.Modal show={showModal} container={this} onHide={this.onHide}> |
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.
surely there should be a way to avoid using this.
in templates
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.
I don't understand, why should we avoid that? This is ES6 style component. Ref: https://reactjs.org/docs/components-and-props.html
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.
While it is possible to use this
as a prop, it isnt desirable for normal usage where it is possible to declare the properties which are available to the template.
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.
I am a bit confused. Are you talking about container={this}
or onHide={this.onHide}
? But both of them are consistent with the react-bootstrap documentation: https://react-bootstrap.github.io/components/modal/#modals-contained
Do you mean onHide
function should be declared in render()
function?
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.
this.onHide
is okay, it's a common pattern https://reactjs.org/docs/handling-events.html . React template isn't a template, each element is transformed into a function call, it's possible to write component without jsx or "template", with pure function call.
I'm not sure about container prop, but I think that's part of Bootstrap.
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.
Ok. I defer to you on this one then.
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.
I note that in most other cases this.
is not being used; this patch is removing many of them, and very rarely adding them.
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 the time this.props
is changed into props
because React.createClass
is changed into functional component. So now props
is being passed as an argument.
Ref: https://reactjs.org/docs/components-and-props.html#functional-and-class-components
src/components/colored-icon.jsx
Outdated
<span className={className} style={headerStyle} title={name}>{children}</span> | ||
); | ||
return ( | ||
<span className={className} style={headerStyle} title={name}>{children}</span> |
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.
four space indent?
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.
Thx, done.
Automated fastforward with GitMate.io was successful! 🎉 |
Refactor deprecated code and upgrade to React v16.3
Closes #16