-
Notifications
You must be signed in to change notification settings - Fork 974
display different payment options based on user location #3862
Conversation
@jkup - sorry for the delay in replying, it's been a little busy this week. first, make sure that you are seeing second, if the code is "US", there will not be any third, if the code isn't "US", and there aren't any thanks! |
Is this needed for 0.12.1? This needs a rebase btw and can't be cleanly merged. |
i would prefer to see it in 0.12.1 ... however, it isn't essential. @jkup - can you construct a clean branch from master and then generate a new PR? thanks, /mtr |
abc79df
to
403987e
Compare
@mrose17 I updated the PR so it doesn't have conflicts. I think there is a misunderstanding though in what state data I have access to. I don't see this.props.ledgerInfo only this.props.ledgerData and I don't see countrycode on that. Do I have access to any of this info in preferences.js? |
@jkup - please check with @diracdeltas ... in to verify what
and look for a series of lines that starts with
good luck! |
@mrose17 i don't see exchangeInfo either when i run with LEDGER_CLIENT_DEBUG=true
|
403987e
to
b715ad8
Compare
</div> | ||
<div className='settingsPanelDivider'> | ||
<a target='_blank' className='browserButton primaryButton' href={exchangeInfo.get('exchangeURL')}> | ||
{exchangeInfo.get('exchangeURL')} |
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 be labeled exchangeInfo.get('exchangeName')
if it's defined
b715ad8
to
5c7e49a
Compare
get exchangePanel () { | ||
const exchangeInfo = this.props.ledgerData.get('exchangeInfo') | ||
const url = exchangeInfo.get('exchangeURL') | ||
const name = exchangeInfo.get('exchangeName') |
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 causes the Add funds
panel to throw an error and not appear at all if exchangeInfo
is undefined. better to use this.props.ledgerData.getIn(['exchangeInfo', propertyname])
5c7e49a
to
f6c262f
Compare
@diracdeltas TIL about .getIn 👍 |
</div> | ||
<div className='settingsPanelDivider'> | ||
<a target='_blank' className='browserButton primaryButton' href={url}> | ||
{name} |
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.
github somehow lost my comment but i think this should say Go to ${name}
instead of just the name
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.
Nice catch! I totally forgot to add the word 'visit' before the button like @bradleyrichter has in his designs! Fixed.
f6c262f
to
0a69a3f
Compare
From Japan, visit Cubits 👍 |
For QA: test needed by who lives in USA |
git rebase -i
to squash commits (if needed).Fix #3711
Test Plan:
I'm having a tough time testing this one. I don't see exchangeInfo.exchangeName or exchangeInfo.exchangeURL even when I VPN to another country. @mrose17 what triggers those to come down? How can I test getting them?
Based on that I might need to change the naming of some of my functions but this is the general idea.