Skip to content
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

Feature/PL-bug-fixes #388

Merged
merged 8 commits into from
May 9, 2018
Merged

Feature/PL-bug-fixes #388

merged 8 commits into from
May 9, 2018

Conversation

plondon
Copy link
Contributor

@plondon plondon commented May 7, 2018

Description

Add a concise explanation of the changes.

Change Type

Please enter one or more of the following:

  • Feature
  • Bug Fix
  • CI/CD
  • Upgrades
  • Other (add explanation)

Testing Steps

Detail the steps required for the reviewer(s) to verify and test these changes.

PR Creator Checklist

  • Code compiles correctly
  • No lint issues exist (verified via yarn lint)
  • All unit tests pass (verified via yarn test)
  • Updated README.md and other documentation (if necessary)

PR Reviewer Checklist

  • Change validated and application spot checked
  • Code styles and best practices met
  • Code is readable and commented when necessary

@plondon plondon added the don't merge yet Code should not be merged label May 8, 2018
@plondon plondon force-pushed the feature/PL-bug-fixes branch from 092f393 to 9156291 Compare May 8, 2018 19:57
@plondon plondon removed the don't merge yet Code should not be merged label May 8, 2018
@plondon plondon requested a review from sixtedemaupeou May 8, 2018 23:04
@plondon plondon added the don't merge yet Code should not be merged label May 8, 2018
@@ -8,7 +8,7 @@ import SelectBoxBitcoin from './template'

class SelectBoxBitcoinAddresses extends React.PureComponent {
getLabel (coin) {
return `All Bitcoin${coin === 'BCH' ? ' Cash' : ''} Wallets`
return this.props.optional ? 'N/A' : `All Bitcoin${coin === 'BCH' ? ' Cash' : ''} Wallets`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you want to display 'N/A'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For importing external btc addrs, there needs to be an option to deselect if you've made a selection.

}

const mapStateToProps = (state, ownProps) => ({
data: getData(state, ownProps.coin)
data: getData(state, ownProps)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do this, also replace the argument coin with props in the getData function

@@ -19,20 +19,21 @@ export const getData = (state, coin) => {
return map(formatAddress, addressesData)
}

const getAddressesData = (coin) => {
const getAddressesData = (ownProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be on getData used in index.js, not getAddressesData used in selectors.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, also turns out defaultProps are not passed in ownProps so I need to change this.

@plondon plondon removed the don't merge yet Code should not be merged label May 9, 2018
@plondon
Copy link
Contributor Author

plondon commented May 9, 2018

@sixtedemaupeou issues should be fixed now. I add a excludeImported option, since we don't want users importing to imported addresses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants