Reorganize ledgerBackup.js #11344

luixxiul · 2017-10-07T10:51:23Z

Fixes #11332
Closes #11343

Remove obsolete styles
Split l10n strings to force font-weight: strong (On Transifex HTML tags may be deleted by contributors, which should not happen)
Set cursor: text to indicate where you can select the passphrase manually. Since the mouse cursor does not change you often see that the passphrase is not selected, while you though you did.
Set the same styles as syncTab.js to avoid possible style inconsistency
Split font shorthand to ensure that the styles could be cascaded properly. See: https://github.com/Khan/aphrodite#object-key-ordering

Auditors: @cezaraugusto

Test Plan:

Open about:preferences#payments
Enable Payments
Go to the advanced setting
Click Backup your wallet
Make sure that the cursor changes on the passphrase div

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

luixxiul · 2017-10-07T10:53:35Z

app/renderer/components/preferences/payment/ledgerBackup.js

  ledgerBackupContent: {
-    // Align the buttons and keys even when the width of the strings is not equal
-    width: 'max-content',
-    margin: 'auto',


Let's not forget to remove what becomes obsolete.

luixxiul · 2017-10-07T10:57:08Z

app/renderer/components/preferences/payment/ledgerBackup.js


-  ledgerBackupContent__copyKey__key__phrase: {
+    // See syncTab.js


We could create a common component to avoid style inconsistency, like PassPhraseContainer.

codecov-io · 2017-10-07T11:12:40Z

Codecov Report

Merging #11344 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #11344      +/-   ##
==========================================
- Coverage   52.51%   52.47%   -0.04%     
==========================================
  Files         267      267              
  Lines       24982    24983       +1     
  Branches     3997     3997              
==========================================
- Hits        13120    13111       -9     
- Misses      11862    11872      +10

Flag	Coverage Δ
#unittest	`52.47% <0%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
.../payment/addFundsDialog/steps/addFundsBatScreen.js	`68.75% <ø> (ø)`	⬆️
app/renderer/components/preferences/syncTab.js	`17.01% <ø> (ø)`	⬆️
...rer/components/preferences/payment/ledgerBackup.js	`36.84% <0%> (-1%)`	⬇️
js/stores/appStoreRenderer.js	`91.17% <0%> (-8.83%)`	⬇️
app/renderer/components/reduxComponent.js	`84.37% <0%> (-6.25%)`	⬇️
js/stores/windowStore.js	`27.27% <0%> (-0.31%)`	⬇️

codecov-io · 2017-10-07T11:12:46Z

Codecov Report

Merging #11344 into master will increase coverage by <.01%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master   #11344      +/-   ##
==========================================
+ Coverage   52.53%   52.53%   +<.01%     
==========================================
  Files         267      267              
  Lines       24975    24978       +3     
  Branches     3996     3996              
==========================================
+ Hits        13120    13122       +2     
- Misses      11855    11856       +1

Flag	Coverage Δ
#unittest	`52.53% <25%> (ø)`	⬆️

Impacted Files	Coverage Δ
app/renderer/components/preferences/syncTab.js	`17.01% <ø> (ø)`	⬆️
...rer/components/preferences/payment/ledgerBackup.js	`36.84% <0%> (-1%)`	⬇️
.../payment/addFundsDialog/steps/addFundsBatScreen.js	`72.22% <100%> (+3.47%)`	⬆️

cezaraugusto · 2017-10-09T04:50:53Z

app/renderer/components/preferences/payment/addFundsDialog/steps/addFundsBatScreen.js

@@ -18,10 +19,13 @@ class BatWelcomeScreen extends React.Component {
        className={css(styles.batScreen)}
      >
        <AboutPageSectionTitle data-canWrap data-l10n-id='helloBat' />
-        <p data-l10n-id='helloBatText1'
+        <div data-l10n-id='helloBatText1'


any reason why not paragraph?

not really. I just thought I have not seen p many times under app/renderer/components (we have a lot of div and span instead of p) and standardizing would help us to keep consistency.

If it should be left as p, I'll revert them and that's no problem for me :-)

ya not sure why but please revert this part, the current way is not semantic

cezaraugusto

lgtm but please see comment

cezaraugusto · 2017-10-09T13:58:44Z

also heads-up I just merged #11342

@cezaraugusto

Closes #11343 - Remove obsolete styles - Split l10n strings to force `font-weight: strong` - Set `cursor: text` to indicate where you can select the passphrase manually. Since the mouse cursor does not change you often see that the passphrase is not selected. - Set the same styles as `syncTab.js` to avoid possible style inconsistency - Split font shorthand to ensure that the styles could be cascaded properly. See: https://github.com/Khan/aphrodite#object-key-ordering Auditors: @cezaraugusto Test Plan: 1. Open about:preferences#payments 2. Enable Payments 3. Go to the advanced setting 4. Click `Backup your wallet` 5. Make sure that the cursor changes on the passphrase div

luixxiul · 2017-10-09T14:05:34Z

@cezaraugusto reverted! would you please check again and merge this one if it looks good to you?

cezaraugusto

++ thanks!

luixxiul · 2017-10-09T14:14:14Z

@cezaraugusto would you please merge this to 0.19.x and 0.20.x as well? I'm not quite sure how to and feel I would break the branches. thanks!

cezaraugusto · 2017-10-09T14:25:37Z

I'm doing it now

Reorganize ledgerBackup.js

cezaraugusto · 2017-10-09T14:32:32Z

master 1bb8ff1
0.20.x b1f8771
0.19.x 0f118dc

luixxiul · 2017-10-09T14:36:08Z

thanks! 🙏

luixxiul added feature/about-pages feature/rewards labels Oct 7, 2017

luixxiul added this to the 0.19.x (Beta Channel) milestone Oct 7, 2017

luixxiul self-assigned this Oct 7, 2017

luixxiul requested a review from cezaraugusto October 7, 2017 10:51

luixxiul commented Oct 7, 2017

View reviewed changes

luixxiul added the usability label Oct 7, 2017

luixxiul mentioned this pull request Oct 7, 2017

Reorganize ledgerBackup.js #11343

Closed

luixxiul commented Oct 7, 2017

View reviewed changes

luixxiul mentioned this pull request Oct 7, 2017

Add volatility text to BAT screen #11342

Merged

8 tasks

cezaraugusto reviewed Oct 9, 2017

View reviewed changes

luixxiul mentioned this pull request Oct 9, 2017

Brave wallet backup key overflows #11292

Closed

cezaraugusto approved these changes Oct 9, 2017

View reviewed changes

cezaraugusto merged commit 1bb8ff1 into brave:master Oct 9, 2017

cezaraugusto deleted the polish-addfunds-dialog branch October 9, 2017 14:06

cezaraugusto added a commit that referenced this pull request Oct 9, 2017

Merge pull request #11344 from luixxiul/polish-addfunds-dialog

b1f8771

Reorganize ledgerBackup.js

cezaraugusto added a commit that referenced this pull request Oct 9, 2017

Merge pull request #11344 from luixxiul/polish-addfunds-dialog

0f118dc

Reorganize ledgerBackup.js

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize ledgerBackup.js #11344

Reorganize ledgerBackup.js #11344

luixxiul commented Oct 7, 2017 •

edited

Loading

luixxiul Oct 7, 2017

luixxiul Oct 7, 2017

codecov-io commented Oct 7, 2017

codecov-io commented Oct 7, 2017 •

edited

Loading

cezaraugusto Oct 9, 2017

luixxiul Oct 9, 2017

cezaraugusto Oct 9, 2017

cezaraugusto left a comment

cezaraugusto commented Oct 9, 2017

luixxiul commented Oct 9, 2017

cezaraugusto left a comment

luixxiul commented Oct 9, 2017

cezaraugusto commented Oct 9, 2017

cezaraugusto commented Oct 9, 2017

luixxiul commented Oct 9, 2017


		ledgerBackupContent__copyKey__key__phrase: {
		// See syncTab.js

Reorganize ledgerBackup.js #11344

Reorganize ledgerBackup.js #11344

Conversation

luixxiul commented Oct 7, 2017 • edited Loading

luixxiul Oct 7, 2017

Choose a reason for hiding this comment

luixxiul Oct 7, 2017

Choose a reason for hiding this comment

codecov-io commented Oct 7, 2017

Codecov Report

codecov-io commented Oct 7, 2017 • edited Loading

Codecov Report

cezaraugusto Oct 9, 2017

Choose a reason for hiding this comment

luixxiul Oct 9, 2017

Choose a reason for hiding this comment

cezaraugusto Oct 9, 2017

Choose a reason for hiding this comment

cezaraugusto left a comment

Choose a reason for hiding this comment

cezaraugusto commented Oct 9, 2017

luixxiul commented Oct 9, 2017

cezaraugusto left a comment

Choose a reason for hiding this comment

luixxiul commented Oct 9, 2017

cezaraugusto commented Oct 9, 2017

cezaraugusto commented Oct 9, 2017

luixxiul commented Oct 9, 2017

luixxiul commented Oct 7, 2017 •

edited

Loading

codecov-io commented Oct 7, 2017 •

edited

Loading