Gas price dropdown #61
Conversation
common/components/Header/index.jsx
Outdated
| @@ -1,6 +1,7 @@ | |||
| // @flow | |||
| import React, { Component } from 'react'; | |||
| import Navigation from './components/Navigation'; | |||
| import GasPriceDropdown from './components/GasPriceDropdown.jsx'; | |||
common/reducers/config.js
Outdated
| nodeSelection: Object.keys(NODES)[0] | ||
| nodeSelection: Object.keys(NODES)[0], | ||
| gasPriceGwei: 21, | ||
| gasPriceMaxGwei: 60, |
There was a problem hiding this comment.
if min/max values do not change during app lifetime, we probably dont want to have them in state
There was a problem hiding this comment.
You also need to add new fields to type State
There was a problem hiding this comment.
Good catch with the types! I'll make that change.
Yep, min/max values shouldn't change, so I'll move them out of state. I'll make them constants in the component, unless there's a better place for them to live 😄
| import './GasPriceDropdown.scss'; | ||
|
|
||
| export default class GasPriceDropdown extends Component { | ||
| constructor(props) { |
There was a problem hiding this comment.
Looks like i am the only one, but i think code w/o constructors (where they not needed) are easier to read.
Here you can delete whole consturctor thing and replace it with
export default class GasPriceDropdown extends Component { state = { expanded: false }
There was a problem hiding this comment.
Yep, I definitely agree that's easier to read. I'll add that in 😄
|
|
||
| render() { | ||
| // col-xs-4 applies padding not observed in MEWv3, this overrides | ||
| const paddingReset = { paddingLeft: '0px', paddingRight: '0px' }; |
There was a problem hiding this comment.
You've got an scss file, it would be better served to do styling in there.
There was a problem hiding this comment.
Totally agree the scss file is underutilized at the moment. I'll move the styling over as suggested 😄
| {this.state.expanded && | ||
| <ul | ||
| className="dropdown-menu" | ||
| style={{ padding: '.5rem', minWidth: '300px' }} |
| whiteSpace: 'normal', | ||
| fontWeight: '300', | ||
| margin: '2rem 0 0' | ||
| }} |
| @@ -0,0 +1,17 @@ | |||
| @import "../../../../common/sass/variables"; | |||
There was a problem hiding this comment.
You can just to @import "common/sass/variables";, it looks from the top of the repo.
There was a problem hiding this comment.
Sounds good! Webstorm was throwing a warning with this path, but indeed it seems to work fine 👍
| }; | ||
|
|
||
| updateGasPrice = (e: SyntheticInputEvent) => { | ||
| this.props.changeGasPrice(e.currentTarget.valueAsNumber); |
There was a problem hiding this comment.
If you call a prop function without checking for it first, you'll need to make the propType PropTypes.func.isRequired
There was a problem hiding this comment.
lets get rid of proptypes :D
There was a problem hiding this comment.
Looks like there's a Babel plugin for that 😀
https://www.npmjs.com/package/babel-plugin-transform-react-remove-prop-types
There was a problem hiding this comment.
Yeah they do nothing in production anyway.
|
@crptm & @wbobeirne, I think I addressed both of your comments with this latest round of commits. Let me know if I missed anything 👍 |
| }; | ||
|
|
||
| render() { | ||
| const gasPriceMinGwei = 1; |
There was a problem hiding this comment.
you can hoist it to module OR move to config/{something}
common/components/Header/index.jsx
Outdated
| @@ -47,6 +50,11 @@ export default class Header extends Component { | |||
| Open-Source & Client-Side Ether Wallet · v3.6.0 | |||
| </span> | |||
|
|
|||
| <GasPriceDropdown | |||
| gasPriceGwei={this.props.gasPriceGwei} | |||
| changeGasPrice={this.props.changeGasPrice} | |||
There was a problem hiding this comment.
Would prefer value & onChange here
|
🚢 it |
Feature/new paper wallet layout
Implements the gas price slider in the header. I'm very new to React, Redux, and Flow, so please point out anything silly I might have done 😄