-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Loading network screen #5893
Loading network screen #5893
Conversation
04e411d
to
9b4d61a
Compare
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.
A couple of small nitpicky things. Also should we write LoadingNetworkError
as a class for consistency sake?
constructor (props) { | ||
super(props) | ||
|
||
this.state = { |
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 can move this out of the ctor and onto the class as a property
import Spinner from '../spinner' | ||
import Button from '../button' | ||
|
||
class LoadingNetworkScreen 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.
Can we use PureComponent
here?
} | ||
} | ||
|
||
LoadingNetworkScreen.propTypes = { |
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.
Can we can move this into the class a property?
@@ -0,0 +1,2 @@ | |||
const LoadingNetworksScreen = require('./loading-network-screen.container') | |||
module.exports = LoadingNetworksScreen |
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.
Why did you choose to use module.exports
over export
here?
|
||
} | ||
|
||
module.exports = LoadingNetworkScreen |
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.
Can we use export default
here?
} | ||
} | ||
|
||
LoadingNetworkScreen.propTypes = { |
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.
Let's use static propTypes
in the class definition similarly to contextTypes
.
import Spinner from '../spinner' | ||
import Button from '../button' | ||
|
||
class LoadingNetworkScreen 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.
Can we extend PureComponent
to reduce rendering assuming the props aren't being mutated?
t: PropTypes.func, | ||
} | ||
|
||
componentWillMount = () => { |
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.
Can we use componentDidMount
here instead, since componentWillMount
will be deprecated in the future?
showNetworkDropdown() | ||
}} | ||
> | ||
{ 'Switch Networks' } |
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.
Should this and "Try Again" below be translated strings?
submitText={t('tryAgain')} | ||
> | ||
<ModalContent | ||
description={'Oops! Something went wrong.'} |
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.
Should this be translated text?
@@ -40,7 +40,7 @@ export default class TransactionList extends PureComponent { | |||
shouldShowRetry = transaction => { | |||
const { transactionToRetry } = this.props | |||
const { id, submittedTime } = transaction | |||
return id === transactionToRetry.id && Date.now() - submittedTime > 30000 | |||
return id === transactionToRetry.id && Date.now() - submittedTime > 3000 |
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.
Should this change be committed? Or was the time reduced to 3 seconds for testing purposes?
ui/app/selectors.js
Outdated
const { metamask: { provider: { type, nickname, rpcTarget } } } = state | ||
|
||
return nickname || rpcTarget || type | ||
|
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.
nit: remove newline
caa69be
to
6e89fee
Compare
6e89fee
to
ad1ddc6
Compare
@bdresser the translation issue has been fixed |
@alextsg the other translation you pointed out "Oops! Something Went Wrong" has also been fixed |
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.
One small issue (I think)
@@ -1215,6 +1221,8 @@ | |||
}, | |||
"speedUpTransaction": { | |||
"message": "Speed up this transaction" | |||
"switchNetworks": { |
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 looks like a formatting 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 think there should be a closing brace on the line above
ad1ddc6
to
04cc98d
Compare
fixes #3776
fixes #5609