Refactor syncTab.js with Aphrodite #8044

luixxiul · 2017-04-02T19:15:06Z

Closes #8042
Addresses #7989
Addresses #8054
Addresses #8587

Unified the modal dialog design of payments and sync
- Copied the styles of modal dialog of payments to commonStyles.js, adding modalOverlay__dialog and modalOverlay__title to ModalOverlay on syncTab.js
- Copied panelMargin, panelItemMargin, and panelPadding from payment.js to global.js
- Added modalVeryLightGray, modalLightGray, and borderRadiusModal to global.js
- Replaced linkButton with primaryButton
- Set flex:1 to two elements under postSetupContent
- TODO: refactor modalOverlay with Aphrodite to remove !important from syncTab.js and commonStyles.js
Introduced modalOverlay__footer and modalOverlay__footerButton
- Removed advanceFooter and replaced paymentCommon.advanceFooter with them to make the styling of the modal dialog footer consistent
Removed styles under .syncContainer, #syncPassphrase, and #syncQR in preferences.less
- Unified margins between buttons on dialog to 8px with globalStyles.spacing.overlayButtonMargin
  Aligned buttons to the right
Removed .formControl from forms.less
- We no longer need it as commonStyles.formControl is applied manually, along with commonStyles.textbox, commonStyles.textbox__outlineable, and commonStyles.textbox__isSettings.

Also:

Changed settingsListContainerMargin into a variable to apply it to syncOverlayBody__form
Added "Done" button to the new device sync dialog
Replaced 'sync' with 'Sync'
Updated test code

Auditors:

Test plan 1:

Open about:preferences#sync
Verify you are able to sync two devices using the secret code
Visit a site on device 1 and change shield setting, ensure that the saved site preference is synced to device 2
Enable Browsing history sync on device 1, ensure the history is shown on device 2
Import/Add bookmarks on device 1, ensure it is synced on device 2
Ensure imported bookmark folder structure is maintained on device 2
Ensure bookmark favicons are shown after sync
Open about:preferences#payments
Click "Add funds..."
Make sure the style of the modal overlay dialog is same as that of sync dialog

Test Plan 2:

Automated tests should pass
Open about:preferences#sync
Make the window small
Scroll the page to the bottom
Make sure there is 2rem margin between the reset sync button and the the border of the window

Test Plan 3:

Open about:preferences#sync
Open about:preferences#payments
Make sure the headers appear exactly on the same place

Test Plan 4:

Reset Sync
Make sure margin-top of the buttons is set to 18px
Enable Sync
Make sure margin-top of DefaultSectionTitle is set to 2rem
Disable Sync
Make sure margin around the gray box is set to 15px

Test Plan 5:

Enable Sync
Click "Sync a new device"
Click "Show secret QR code"
Hover on the QR image
Make sure the title appears

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).

luixxiul · 2017-04-02T19:26:54Z

Requesting a design review from @bradleyrichter: #8044 (comment)

luixxiul · 2017-04-03T05:24:38Z

@ayumi @diracdeltas Should "sync" be "Sync" except "sync code"? ie "Set up Sync" and "I am new to Sync"

ayumi · 2017-04-03T07:23:12Z

@luixxiul "Sync" is correct, and i think "Sync code" too– like entering your Sync code words.

cezaraugusto · 2017-04-17T17:50:57Z

sorry why is PR marked as blocked?

luixxiul · 2017-04-17T17:52:12Z

because it requires #8237 ("Brave Sync" header issue). See: Test Plan 3.

NejcZdovc · 2017-04-30T09:03:33Z

@luixxiul I think this is a good time, because we are refactoring to aphrodite and we are leaving leftovers that don't do anything. I searched for every class in the less file and it's not there. Can you double check and if you don't find it in our code base I would suggest to just delete it in this PR

NejcZdovc · 2017-04-30T09:05:53Z

This way we keep our code clean and up to date, otherwise we have dead code and it's hard to track what needs to be removed and what not from a third person after a month or so.

cc @bsclifton

luixxiul · 2017-04-30T09:06:01Z

Fine, if so would you mind searching unnecessary classes and ids of #8443

luixxiul · 2017-04-30T09:46:10Z

@NejcZdovc done, review again. thanks

bsclifton · 2017-04-30T19:15:04Z

++ on removing the dead code- thanks @luixxiul 😄

NejcZdovc

Looking really good, just few more small fixes and we are good to go

NejcZdovc · 2017-04-30T20:29:32Z

app/renderer/components/preferences/syncTab.js

-      syncOverlay: true,
-      [css(styles.syncOverlayBody__wrapper)]: true
-    })}>
+    return <div className={css(styles.syncOverlayBody__wrapper)}>
      <div className={css(styles.syncOverlayBody__label)} data-l10n-id='syncEnterPassphrase' />
      <div className={css(styles.syncOverlayBody__form)}>
        <textarea className={cx({


We don't need cx here anymore

NejcZdovc · 2017-04-30T20:31:45Z

app/renderer/components/preferences/syncTab.js

+        </ul>
+        <img className={css(styles.syncOverlayBody__syncQRImg)}
+          src={this.props.syncData.get('seedQr')}
+          title='Brave sync QR code'


Can you please translate this?

that's the another task but I will

NejcZdovc · 2017-04-30T20:32:33Z

app/renderer/components/preferences/syncTab.js

-        placeholder={this.defaultDeviceName} />
+      <div className={css(styles.syncOverlayBody__label)} data-l10n-id='syncDeviceNameInput' />
+      <div>
+        <input className={cx({


We don't need cx here anymore

NejcZdovc · 2017-04-30T20:32:54Z

app/renderer/components/preferences/syncTab.js

+    return <div className={css(styles.syncOverlayBody__wrapper)}>
+      <div className={css(styles.syncOverlayBody__label)} data-l10n-id='syncEnterPassphrase' />
+      <div className={css(styles.syncOverlayBody__form)}>
+        <textarea className={cx({


We don't need cx here anymore

Closes #8042 Addresses #7989 Addresses #8054 - Unified the modal dialog design of payments and sync Copied the styles of modal dialog of payments to commonStyles.js, adding modalOverlay__dialog and modalOverlay__title to ModalOverlay on syncTab.js Copied panelMargin, panelItemMargin, and panelPadding from payment.js to global.js Added modalVeryLightGray, modalLightGray, and borderRadiusModal to global.js Replaced linkButton with primaryButton Set flex:1 to two elements under postSetupContent TODO: refactor modalOverlay with Aphrodite to remove !important from syncTab.js and commonStyles.js - Introduced modalOverlay__footer and modalOverlay__footerButton Removed advanceFooter and replaced paymentCommon.advanceFooter with them to make the styling of the modal dialog footer consistent - Removed styles under .syncContainer, #syncPassphrase, and #syncQR in preferences.less Unified margins between buttons on dialog to 8px with globalStyles.spacing.overlayButtonMargin Aligned buttons to the right - Removed .formControl from forms.less We no longer need it as commonStyles.formControl is applied manually, along with commonStyles.textbox, commonStyles.textbox__outlineable, and commonStyles.textbox__isSettings. Also: - Changed settingsListContainerMargin into a variable to apply it to syncOverlayBody__form - Added "Done" button to the new device sync dialog - Added styles.section to unify the margin-bottom of each section - Replaced 'sync' with 'Sync' - Updated test code Auditors: Test plan 1: 1. Open about:preferences#sync 2. Verify you are able to sync two devices using the secret code 3. Visit a site on device 1 and change shield setting, ensure that the saved site preference is synced to device 2 4. Enable Browsing history sync on device 1, ensure the history is shown on device 2 5. Import/Add bookmarks on device 1, ensure it is synced on device 2 6. Ensure imported bookmark folder structure is maintained on device 2 7. Ensure bookmark favicons are shown after sync 8. Open about:preferences#payments 9. Click "Add funds..." 10. Make sure the style of the modal overlay dialog is same as that of sync dialog Test Plan 2: 1. Automated tests should pass 2. Open about:preferences#sync 3. Make the window small 4. Scroll the page to the bottom 5. Make sure there is 2rem margin between the reset sync button and the the border of the window Test Plan 3: 1. Open about:preferences#sync 2. Open about:preferences#payments 3. Make sure the headers appear exactly on the same place Test Plan 4: 1. Reset Sync 2. Make sure margin-top of the buttons is set to 18px 3. Enable Sync 4. Make sure margin-top of DefaultSectionTitle is set to 2rem 5. Disable Sync 6. Make sure margin around the gray box is set to 15px Test Plan 5: 1. Enable Sync 2. Click "Sync a new device" 3. Click "Show secret QR code" 4. Hover on the QR image 5. Make sure the title appears

luixxiul · 2017-05-01T02:35:36Z

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

@@ -74,7 +74,7 @@ class LedgerBackupFooter extends ImmutableComponent {
  }

  render () {
-    return <div className={css(paymentCommon.advanceFooter)}>
+    return <div className={css(commonStyles.modalOverlay__footer, commonStyles.modalOverlay__footerButton)}>


Actually this kind of styling will be removed with the PR on refactoring modalOverlay, which I will submit later.

luixxiul · 2017-05-01T04:30:57Z

Also asking @srirambv for manual test plans. thanks in advance.

NejcZdovc

LGTM

srirambv · 2017-05-01T10:13:01Z

@luixxiul popup still needs to be fixed. Buttons alligned next to the text. Should be on the next line

srirambv

popup confirmation modal needs rework. Buttons are aligned next to text. Should be below the texr

luixxiul · 2017-05-01T10:26:00Z

popup confirmation modal needs rework. Buttons are aligned next to text. Should be below the texr

@srirambv I confirmed that bug appeared also on the master branch. Let's address the issue with another PR.

Except that, looking good?

srirambv · 2017-05-01T10:34:59Z

@luixxiul Is the colour of the sync passphrase expected to chang from #ff0400 to D44600?

luixxiul · 2017-05-01T10:38:11Z

@srirambv Yes it is. color: globalStyles.color.braveDarkOrange

From this random color: https://github.com/brave/browser-laptop/pull/8044/files#diff-4b50ff14dadc9756719a577a72f520d6L653

srirambv · 2017-05-01T11:13:00Z

@luixxiul I can open up an issue for the confirmation modal for a new PR. Apart from that everything else looks good 👍

luixxiul added feature/sync refactoring/aphrodite labels Apr 2, 2017

luixxiul self-assigned this Apr 2, 2017

luixxiul added the PR/blocked label Apr 2, 2017

luixxiul requested a review from bradleyrichter April 2, 2017 19:26

luixxiul mentioned this pull request Apr 2, 2017

Unify modal dialogs of payments and sync #8042

Closed

luixxiul added PR/work-in-progress ⚒ and removed PR/work-in-progress ⚒ labels Apr 3, 2017

luixxiul mentioned this pull request Apr 5, 2017

Use <SettingTextbox> for the textbox on the dialog of enabling Sync #7360

Closed

luixxiul changed the base branch from master to refactoring-aphrodite April 6, 2017 09:22

luixxiul added the PR/needs-rebase ۞ label Apr 6, 2017

luixxiul requested a review from NejcZdovc April 6, 2017 15:36

luixxiul removed the PR/needs-rebase ۞ label Apr 6, 2017

luixxiul mentioned this pull request Apr 7, 2017

Refactor modalOverlay with Aphrodite #8054

Closed

luixxiul changed the base branch from refactoring-aphrodite to master April 10, 2017 03:43

luixxiul requested review from cezaraugusto and bsclifton April 10, 2017 14:49

luixxiul added this to the 0.14.3 milestone Apr 10, 2017

luixxiul added PR/work-in-progress ⚒ PR/blocked and removed PR/blocked PR/work-in-progress ⚒ labels Apr 16, 2017

luixxiul mentioned this pull request Apr 16, 2017

Create sectionTitle.js #8237

Merged

4 tasks

luixxiul removed the request for review from NejcZdovc April 22, 2017 05:53

luixxiul added the PR/work-in-progress ⚒ label Apr 22, 2017

luixxiul mentioned this pull request Apr 30, 2017

Remove unnecessary classes and ids from files converted with Aphrodite #8587

Closed

NejcZdovc reviewed Apr 30, 2017

View reviewed changes

luixxiul commented May 1, 2017

View reviewed changes

luixxiul requested a review from srirambv May 1, 2017 04:29

NejcZdovc approved these changes May 1, 2017

View reviewed changes

srirambv suggested changes May 1, 2017

View reviewed changes

luixxiul removed the request for review from bradleyrichter May 1, 2017 10:42

srirambv approved these changes May 1, 2017

View reviewed changes

luixxiul merged commit b831100 into brave:master May 1, 2017

luixxiul removed the PR/needs-QA-attention ☕ label May 1, 2017

luixxiul removed request for cezaraugusto and bsclifton May 1, 2017 11:15

luixxiul deleted the sync-aphrodite branch May 1, 2017 11:15

srirambv mentioned this pull request May 1, 2017

Switch and buttons on messageBox #8598

Closed

This was referenced May 5, 2017

Generate Release Notes for 0.15.3 #8617

Closed

Per-release issue test for OS X 0.15.3 #8619

Closed

Manual tests for Windows x64 0.15.3 #8725

Closed

Manual tests for Linux 0.15.3 #8726

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor syncTab.js with Aphrodite #8044

Refactor syncTab.js with Aphrodite #8044

luixxiul commented Apr 2, 2017 •

edited

Loading

luixxiul commented Apr 2, 2017

luixxiul commented Apr 3, 2017

ayumi commented Apr 3, 2017

cezaraugusto commented Apr 17, 2017

luixxiul commented Apr 17, 2017 •

edited

Loading

NejcZdovc commented Apr 30, 2017

NejcZdovc commented Apr 30, 2017

luixxiul commented Apr 30, 2017

luixxiul commented Apr 30, 2017

bsclifton commented Apr 30, 2017

NejcZdovc left a comment

NejcZdovc Apr 30, 2017 •

edited

Loading

NejcZdovc Apr 30, 2017

luixxiul May 1, 2017

NejcZdovc Apr 30, 2017

NejcZdovc Apr 30, 2017

luixxiul May 1, 2017

luixxiul commented May 1, 2017

NejcZdovc left a comment

srirambv commented May 1, 2017

srirambv left a comment

luixxiul commented May 1, 2017 •

edited

Loading

srirambv commented May 1, 2017

luixxiul commented May 1, 2017 •

edited

Loading

srirambv commented May 1, 2017

Refactor syncTab.js with Aphrodite #8044

Refactor syncTab.js with Aphrodite #8044

Conversation

luixxiul commented Apr 2, 2017 • edited Loading

Test plan 1:

Test Plan 2:

Test Plan 3:

Test Plan 4:

Test Plan 5:

luixxiul commented Apr 2, 2017

luixxiul commented Apr 3, 2017

ayumi commented Apr 3, 2017

cezaraugusto commented Apr 17, 2017

luixxiul commented Apr 17, 2017 • edited Loading

NejcZdovc commented Apr 30, 2017

NejcZdovc commented Apr 30, 2017

luixxiul commented Apr 30, 2017

luixxiul commented Apr 30, 2017

bsclifton commented Apr 30, 2017

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc Apr 30, 2017 • edited Loading

Choose a reason for hiding this comment

NejcZdovc Apr 30, 2017

Choose a reason for hiding this comment

luixxiul May 1, 2017

Choose a reason for hiding this comment

NejcZdovc Apr 30, 2017

Choose a reason for hiding this comment

NejcZdovc Apr 30, 2017

Choose a reason for hiding this comment

luixxiul May 1, 2017

Choose a reason for hiding this comment

luixxiul commented May 1, 2017

NejcZdovc left a comment

Choose a reason for hiding this comment

srirambv commented May 1, 2017

srirambv left a comment

Choose a reason for hiding this comment

luixxiul commented May 1, 2017 • edited Loading

srirambv commented May 1, 2017

luixxiul commented May 1, 2017 • edited Loading

srirambv commented May 1, 2017

luixxiul commented Apr 2, 2017 •

edited

Loading

luixxiul commented Apr 17, 2017 •

edited

Loading

NejcZdovc Apr 30, 2017 •

edited

Loading

luixxiul commented May 1, 2017 •

edited

Loading

luixxiul commented May 1, 2017 •

edited

Loading