Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Polish add funds dialog #11291

Closed
wants to merge 6 commits into from
Closed

Polish add funds dialog #11291

wants to merge 6 commits into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Oct 5, 2017

Fixes #11289
Addresses #11193

Auditors: @cezaraugusto

Test Plan:

addfunds

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

NejcZdovc and others added 5 commits October 5, 2017 07:22
Fixes balance and transactions with help of @mrose17

use "the latest and greatest" of bat-*

and also prepare for new anonymizing proxy
- remove contrib panel
- set wallet address as readonly
- set wallet adress as auto selected
- several strings change
- changed coins order in wizard
- small tweaks on design
@luixxiul luixxiul self-assigned this Oct 5, 2017
@luixxiul luixxiul added feature/rewards design A design change, especially one which needs input from the design team. labels Oct 5, 2017
@luixxiul luixxiul added this to the 0.19.x (Beta Channel) milestone Oct 5, 2017
@luixxiul
Copy link
Contributor Author

luixxiul commented Oct 5, 2017

TODO: Change the string copied when you click the clipboard button to Copied! just as you get on about:brave.

Addressed with ab06756#diff-fe64555358df277d4c4c5ed8609eb492R90

@NejcZdovc
Copy link
Contributor

@luixxiul this should do it

diff --git a/app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardAddress.js b/app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardAddress.js
index 75f3a5ec6..06dd07c0c 100644
--- a/app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardAddress.js
+++ b/app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsWizardAddress.js
@@ -40,6 +40,7 @@ class AddFundsWizardAddress extends React.Component {
   get copyToClipboardButton () {
     return (
       <ClipboardButton
+        dataL10nId='copyToClipboard'
         bottomTooltip
         className={globalStyles.appIcons.clipboard}
         copyAction={this.onCopy}

@NejcZdovc
Copy link
Contributor

nope, sorry this is not the correct one

@luixxiul
Copy link
Contributor Author

luixxiul commented Oct 5, 2017

yeah copied is hardcoded.

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Oct 5, 2017

ok found it. We only have it in app.properties, we need it in preferences.properties as well

diff --git a/app/extensions/brave/locales/en-US/preferences.properties b/app/extensions/brave/locales/en-US/preferences.properties
index fec6a1909..4d86a6f4a 100644
--- a/app/extensions/brave/locales/en-US/preferences.properties
+++ b/app/extensions/brave/locales/en-US/preferences.properties
@@ -86,6 +86,7 @@ contributionStatementFooterNoteBoxHeading2=About publisher distributions
 contributionStatements=Contribution statements
 contributionTime=Contribution Time
+copied=Copied!
 copy=Copy
 copyToClipboard=Copy to clipboard
 createdWalletStatus=Your wallet is ready!
 createWallet=create wallet

Fixes #11289
Addresses #11193

Auditors: @cezaraugusto

Test Plan:
@bradleyrichter
Copy link
Contributor

@luixxiul @cezaraugusto The text strings as shown above are not correct. The main strings should be as follows:

Go to your external Bitcoin account and send BTC to your Brave wallet address below:

Go to your external Ethereum account and send ETH to your Brave wallet address below:

Go to your external BAT account and send BAT to your Brave wallet address below:

Go to your external Lightcoin account and send LTC to your Brave wallet address below:


The lower "note:" strings look correct but need a width adjustment.

The layout needs a little more refinement to make everything fit and align.

image

Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

@luixxiul changes Brad suggested are in the main add funds branch, please cherry-pick from there and add your changes. for what I see this lgtm but yes UI changed a little

@luixxiul
Copy link
Contributor Author

luixxiul commented Oct 5, 2017

oh, it looks like I have just reverted the design to the old one...

With the current layout you would not be able to align the textbox with the currency icon, as the icon is added there with :before pseudo-class specified with a parent where the rest of the stuff is placed.

To achieve that:

  • create a block for the icon and the main strings
  • Set width: calc(100% - ${marginAroundTheDivider} - ${qrcodeWidth}) to the block

and

  • Set width: 100% to the groupedTextbox wrapper div

and

  • create a block for note strings
  • Set max-width: calc(${upholdFooterBlockWidth} - calc(${upholdLogoWidth} * 2))
  • Set display: flex and flex: flex-end

and so on...

--

If that re-alignment is not necessary, adding enough margin for Copied! tooltip to fix #11289 should be enough.

@luixxiul luixxiul added needs-info Another team member needs information from the PR/issue opener. help wanted The PR/issue opener needs help to complete/report the task. labels Oct 5, 2017
@luixxiul
Copy link
Contributor Author

luixxiul commented Oct 5, 2017

I'll be offline so help wanted 🙏

@luixxiul luixxiul removed their assignment Oct 5, 2017
@luixxiul
Copy link
Contributor Author

luixxiul commented Oct 5, 2017

Un-assigning myself for now..

If this PR is not necessary anymore, please just close it in favor of a new one, thanks!

@NejcZdovc NejcZdovc force-pushed the bat-integration branch 3 times, most recently from cb2ef1c to f2e92e2 Compare October 6, 2017 13:25
@NejcZdovc
Copy link
Contributor

closing this one for now, because I think @cezaraugusto addressed all feedbacks from @bradleyrichter. If that's not the case please reopen it.

@NejcZdovc NejcZdovc closed this Oct 6, 2017
@NejcZdovc NejcZdovc removed this from the 0.19.x (Beta Channel) milestone Oct 6, 2017
@luixxiul luixxiul deleted the addFunds-dialog-suggestion branch October 6, 2017 14:32
@luixxiul luixxiul removed help wanted The PR/issue opener needs help to complete/report the task. needs-info Another team member needs information from the PR/issue opener. labels Oct 6, 2017
@luixxiul luixxiul removed the request for review from bradleyrichter October 6, 2017 14:32
@luixxiul
Copy link
Contributor Author

luixxiul commented Oct 6, 2017

@cezaraugusto let's do that as a follow-up for 0.21.x or 0.22.x since that is not a blocker. sounds good?

If so, I'm going to open an issue for them.

@luixxiul luixxiul added the needs-info Another team member needs information from the PR/issue opener. label Oct 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design A design change, especially one which needs input from the design team. feature/rewards needs-info Another team member needs information from the PR/issue opener.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants