Skip to content
This repository has been archived by the owner. It is now read-only.

sync v2 #13197

Closed
wants to merge 16 commits into from
Closed

sync v2 #13197

wants to merge 16 commits into from

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Feb 20, 2018

Closes #12269
Closes #9254
Closes #12356

Test Plan:

  • Run npm start and npm run start2 so you can sync between devices
  • You should be able to copy passphrase content using the clipboard button
  • You should be able to Sync two devices by typing the correct passphrase
  • Passphrase should have a counter and it should match the number of words (current default 16)
  • Once you sync both devices, they should appear in the postSetup content (inside sortableTable)

Note for reviewers: There's a bug reported in #13258 where you can't sync devices again once you reset sync. That will be tracked separately so in case you're testing multiple options, ensure to clear your profile after every reset you want.

Screenshots UPDATED:

screen shot 2018-04-04 at 12 00 34 am

screen shot 2018-04-04 at 12 00 30 am

screen shot 2018-04-04 at 12 00 18 am

screen shot 2018-04-04 at 12 00 10 am

screen shot 2018-04-03 at 11 56 19 pm

screen shot 2018-04-03 at 11 56 14 pm

screen shot 2018-04-03 at 11 56 10 pm

screen shot 2018-04-03 at 11 56 06 pm

@cezaraugusto cezaraugusto self-assigned this Feb 20, 2018
@cezaraugusto cezaraugusto added this to the 0.21.x (Beta Channel) milestone Feb 20, 2018
@alexwykoff alexwykoff modified the milestones: 0.21.x (Beta Channel), 0.22.x (Developer Channel) Feb 20, 2018
@cezaraugusto cezaraugusto force-pushed the sync/v2/12269 branch 3 times, most recently from 25e4a30 to 75c689d Compare Feb 22, 2018
@bbondy bbondy modified the milestones: 0.22.x (Developer Channel), 0.23.x (Nightly Channel) Feb 25, 2018
@@ -347,10 +363,15 @@ syncResetMessageOtherDevices=If you've synced other devices, they will continue
syncResetMessageWhat=Resetting Sync clears data stored on the Sync server and resets this device's Sync settings.
syncResetMessageWhatNot=You will keep any bookmarks, history and other browsing data currently on this device.
syncRetryButton=Try again
syncScan=Scan the code
syncWelcome1=Brave Sync allows you to sync bookmarks, tabs, history and other data privately between your Brave Browsers on your varios devices.
Copy link
Member

@diracdeltas diracdeltas Feb 26, 2018

Choose a reason for hiding this comment

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

various

@@ -1015,7 +1015,8 @@ module.exports.defaultAppState = () => {
lastFetchTimestamp: 0,
objectsById: {},
pendingRecords: {},
lastConfirmedRecordTimestamp: 0
lastConfirmedRecordTimestamp: 0,
setupCompleted: false
Copy link
Member

@diracdeltas diracdeltas Feb 26, 2018

Choose a reason for hiding this comment

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

please add this to docs/state.md

Copy link
Contributor Author

@cezaraugusto cezaraugusto Mar 1, 2018

Choose a reason for hiding this comment

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

done 👍

package.json Outdated
"run": [
"lint",
"pre-push-tests"
]
Copy link
Member

@diracdeltas diracdeltas Feb 26, 2018

Choose a reason for hiding this comment

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

should this have been removed? (btw you can delete the commit hooks in .git/hooks instead so it doesn't change any files that are checked into git)

Copy link
Contributor Author

@cezaraugusto cezaraugusto Mar 1, 2018

Choose a reason for hiding this comment

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

ya shouldn't be removed. added back

@diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Feb 26, 2018

is this blocked on #13313?

return
}
}
window.alert('Invalid input code; please try again or create a new profile.')
}

render () {
console.log(JSON.stringify(this.props.syncData.get('devices')))
Copy link
Member

@diracdeltas diracdeltas Feb 26, 2018

Choose a reason for hiding this comment

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

extra console.log

Copy link
Contributor Author

@cezaraugusto cezaraugusto Mar 1, 2018

Choose a reason for hiding this comment

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

oh noes! fixed

@diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Feb 26, 2018

UI comments:

It doesn't seem to update the word count (stays at 1 no matter what i type)
screen shot 2018-02-26 at 11 17 21 am

If I click 'Start new sync chain' > 'Computer', copy the code, then click 'Done', it still shows the pre-sync preferences screen. Is that expected? It only shows the post-sync screen once another device is synced.

On the post-sync preferences screen, if I click 'Sync a new device..., 'Computer', then click the 'X' button to close the window, it goes back to showing the pre-sync screen. This seems like a bug.
screen shot 2018-02-26 at 11 20 58 am

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Mar 1, 2018

ref #13197 (comment)

It doesn't seem to update the word count (stays at 1 no matter what i type)

I'll fix it. I only tested with copy/paste, which works but should work with any user input

If I click 'Start new sync chain' > 'Computer', copy the code, then click 'Done', it still shows the pre-sync preferences screen. Is that expected? It only shows the post-sync screen once another device is synced.

I'm not sure if I followed right but you can't hit done until you go to the next screen (devices list). If you hit the X (close) button you're expected to go to the start screen. The only way to activate Sync is to hit done on the last screen. (there are changes coming on this after Brad feedback).

On the post-sync preferences screen, if I click 'Sync a new device..., 'Computer', then click the 'X' button to close the window, it goes back to showing the pre-sync screen. This seems like a bug.

Ya that's a bug. Addressed in 260ba3e

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Mar 1, 2018

removing reviewers and setting as work in progress to apply new UI changes per Brad request

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Apr 10, 2018

reviews request addressed, left a question regarding seed but per #13197 (review) that's a non-blocker.

Screenshots for passphrase warning added in #13197 (comment). We're ready for a re-review

@diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Apr 10, 2018

@cezaraugusto looks good but the QR code is also private and should show a warning on this screen

screen shot 2018-04-10 at 3 17 48 pm

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Apr 11, 2018

Added notice for QR code screen per #13197 (comment):

screen shot 2018-04-11 at 11 26 40 am

Phrase is the same as other screens cc @bradleyrichter if this one needs adjustment

@@ -563,6 +565,9 @@ const handleAppAction = (action) => {
if (device.name) {
appState = appState.setIn(['sync', 'devices', deviceId, 'name'], device.name)
}
if (!hasMainDevice && getSetting(settings.SYNC_DEVICE_NAME) === device.name) {
Copy link
Member

@diracdeltas diracdeltas Apr 11, 2018

Choose a reason for hiding this comment

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

i think this line just sets the mainDevice to be true for any device with the same name as the current device, since SYNC_DEVICE_NAME is the name of the current device. is that the intended behavior?

Copy link
Member

@diracdeltas diracdeltas Apr 11, 2018

Choose a reason for hiding this comment

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

(as long as the current device doesn't know of any existing main devices)

Copy link
Member

@diracdeltas diracdeltas left a comment

looks good! two follow up issues:

brave/sync#201
brave/sync#202

@bsclifton bsclifton modified the milestones: 0.23.x (Developer Channel), 0.24.x (Nightly Channel) May 1, 2018
@bsclifton
Copy link
Member

@bsclifton bsclifton commented Jun 18, 2018

Great job on this PR - I know you've already shifted work towards Brave Core. I captured what has been done so far with brave/brave-browser#356 for when we do chose to implement this 😄

@bsclifton bsclifton closed this Jun 18, 2018
@bsclifton bsclifton removed this from the 0.24.x (Developer Channel) milestone Jun 18, 2018
@bsclifton bsclifton deleted the sync/v2/12269 branch Aug 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants