Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync v2 #1019

Merged
merged 6 commits into from Dec 12, 2018
Merged

Sync v2 #1019

merged 6 commits into from Dec 12, 2018

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Dec 4, 2018

fix brave/brave-browser#2253
fix brave/brave-browser#2131
fix brave/brave-browser#2128
fix brave/brave-browser#2123

Auditors: @AlexeyBarabash @darkdh
cc @petemill for front-end code review

Notes: Sync v2 flow changed:

  • Users can't type device name anymore;
  • You can only start with a sync chain w/ at least 2 devices;
  • A chain with less than 2 devices will be reset.

Test Plan:

Adding/Syncing devices

  1. Have two different devices or open two browser instances w/ different profiles via diffrent user_data_dir_name
  2. Sync words should work. if you are able to test qrcode, it should work too
  3. Bookmarks syncing switch should be on by default. Bookmarking a page should appear in the connected device within 1 minute which is the time sync takes to fetch updates

Removing devices

  1. After being synced with two devices, try removing one
  2. The main device should have a different modal popup than the remote device
  3. If you have only two devices and remove one, Sync should reset

Error cases

No internet

  1. Start Sync in a fresh profile and click either "Start a new sync chain" or "Enter a sync chain code" buttons
  2. Disable internet in your OS (not via devtools)
  3. Go back to the sync screen
  4. You should see a No internet connection. error dialog

Wrong passphrase

  1. Start Sync in a fresh profile and go to "Enter a Sync chain code"
  2. Type anything honest that is not a Sync word i.e. anthony is the boss
  3. You should see a Invalid sync code. error dialog. He's still the boss tho.

Bad behavior from Sync servers

  1. Create a sync chain and ensure you see two devices
  2. Quit Brave
  3. Launch Brave without internet
  4. You should see a Unable to connect to the Sync servers. error dialog

@cezaraugusto cezaraugusto self-assigned this Dec 4, 2018
petemill
petemill previously requested changes Dec 5, 2018
Copy link
Member

@petemill petemill left a comment

@cezaraugusto I've done a first pass on the code, and same comments as #894 wrt the images. If they can be put in brave-ui alongside where they are used, when they are static, that would be best. If not then they could be placed inside this component instead of the generic brave-core/img directory which we are looking to remove. And we at least don't need to specify then in the GRD or c++ since that is all automatically handled via webpack now.

However I would like to test the functionality further. Is there a test plan, at least for QA? I can't see one on the main sync v2 issue.

components/resources/brave_components_resources.grd Outdated Show resolved Hide resolved
components/resources/brave_components_resources.grd Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
browser/ui/webui/brave_webui_source.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash left a comment

Approving because PR does not contain native changes I would reject.

darkdh
darkdh previously requested changes Dec 5, 2018
Copy link
Member

@darkdh darkdh left a comment

screen shot 2018-12-05 at 09 07 10

  1. I have two Mac and I can't tell which is which
  2. Sync Bookmark can not be toggled on and it should be default on

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Dec 12, 2018

@darkdh you can see who they are via main device words. ping @bradleyrichter re #1019 (review) do you think this could be a usability issue?

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Dec 12, 2018

@darkdh @AlexeyBarabash @petemill sync v2 is ready for re-review

@AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Dec 12, 2018

Thanks, @cezaraugusto . Looks fresh comparing to v1.

I am not a designer, but maybe the content should be centered vertically.

image

@AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Dec 12, 2018

@cezaraugusto
I like the button Copy to clipboard when I copy the code from the device in chain.

But when I connect device to existing sync chain, I also have the Copy to clipboard button.
Is it possible to make Paste from clipboard button? Or remove Copy to clipboard when entering sync code.

image

@AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Dec 12, 2018

I also have two Linux devices and cannot distinguish them
image

@cezaraugusto cezaraugusto force-pushed the sync2 branch 2 times, most recently from 8344f9c to 272fc6b Compare Dec 12, 2018
@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Dec 12, 2018

@AlexeyBarabash thanks for reviewing

  • re <textarea> comment I removed the copy option. we don't have the paste feature in brave-ui and wouldn't make for this change anyway
  • re first screen positioning that is following the design spec
  • re the option to distinguish between devices I've bring that up to design in https://bravesoftware.slack.com/archives/C2HJYB45N/p1544587433023800

@srirambv
Copy link
Contributor

@srirambv srirambv commented Dec 12, 2018

@cezaraugusto @AlexeyBarabash @darkdh Is it possible to add a check for unique device names at the time of joining a sync group?

@cezaraugusto cezaraugusto dismissed stale reviews from darkdh and petemill Dec 12, 2018

feedback regarding main device/this device addressed

}
{
scanCode
? <ScanCodeModal syncData={syncData} actions={actions} onClose={this.onClickPhoneTabletButton} />
Copy link
Member

@petemill petemill Dec 12, 2018

Choose a reason for hiding this comment

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

ux nit: we are opening a modal on top of another modal, can't we re-use the same modal? Right now this requires two clicks of the close button to get back to the beginning.

@bradleyrichter
Copy link
Contributor

@bradleyrichter bradleyrichter commented Dec 12, 2018

@cezaraugusto "main device" should be "this device" which will cover MOST cases where the device names are the same.

For linux, are we not able to get the device names?

petemill
petemill previously approved these changes Dec 12, 2018
Copy link
Member

@petemill petemill left a comment

Some ux nits that can be considered for follow-ups.

However, I had an issue with moving a bookmark in to a folder - the other device did not update to move the bookmark. But I guess that is not a problem with the front-end in this PR.

I did also try the no-internet error test and I did not receive an alert at all when I restarted the browser, even though I could not access any web sites in the browser due to no network connection. That is also not in this PR, right @cezaraugusto @darkdh ?

Based on these assumptions, LGTM, but please correct me if I'm wrong.

1

ux nit: table column spacing (and button column could collapse to above / below table)
image

2

ux nit: This line-height feels extremely cramped and uneven
image

3

ux nit: we are opening a modal on top of another modal to start the experience. Should we re-use the same modal? Right now this requires two clicks of the close button to get back to the beginning.

4

ux nit: the toggles seem to have an extra couple of blank rows which I presume is from disabled sync types. But this creates an arbitrary amount of space. If we want the button to have margin then we can specify a margin, but I doubt this is the exact space that we want
image

package.json Outdated
@@ -272,7 +272,7 @@
"@types/react-dom": "^16.0.7",
"@types/react-redux": "6.0.4",
"awesome-typescript-loader": "^5.2.0",
"brave-ui": "brave/brave-ui#cee363dd33ec3c7499b7d239384a6f2eb601f981",
"brave-ui": "github:brave/brave-ui#2b8e852f5585342594bfaf29a2d392ba2fe72b81",
Copy link
Member

@petemill petemill Dec 12, 2018

Choose a reason for hiding this comment

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

We need to remember not to merge this PR until this brave-ui commit is on brave-ui master.
Steps:

  1. get this PR approved
  2. merge brave-ui requirements to brave-ui master
  3. point this PR at latest brave-ui master
  4. merge this PR

@petemill
Copy link
Member

@petemill petemill commented Dec 12, 2018

@bradleyrichter @rossmoody I left some UX feedback above that we should follow-up on

@darkdh
Copy link
Member

@darkdh darkdh commented Dec 12, 2018

@petemill offline should trigger an alert. If you can not see that in this PR, then it is a regression.

bbondy
bbondy previously approved these changes Dec 12, 2018
Copy link
Member

@bbondy bbondy left a comment

carrying forward r+ after rebase, not actually reviewed by me.

@petemill
Copy link
Member

@petemill petemill commented Dec 12, 2018

@kjozwiak @rebron @srirambv just to confirm that this is going on 0.58.x

Copy link
Member

@petemill petemill left a comment

approved brave-ui sha change

@cezaraugusto cezaraugusto merged commit 9dedff7 into master Dec 12, 2018
1 check was pending
@cezaraugusto cezaraugusto deleted the sync2 branch Dec 12, 2018
cezaraugusto added a commit that referenced this issue Dec 12, 2018
cezaraugusto added a commit that referenced this issue Dec 12, 2018
cezaraugusto added a commit that referenced this issue Dec 12, 2018
@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Dec 12, 2018

master 9dedff7
0.60.x 205dbf4
0.59.x ce201a1 (this change) d8e1834 (brave-ui sha update)
0.58.x 1e72e55 (this change) 9b61a05 (brave-ui sha update)

@AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Dec 13, 2018

I cannot start a new chain when this PR was merged.
Have a clean profile.

  1. Press Start a new sync chain
    image
  2. It prompts to choose a new device
    image
  3. I want to choose mobile, but get no QR code
    image
    3-2) I want to choose a desktop, it wants to have a sync chain code
    image

@AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Dec 13, 2018

Cannot see above on mac. Maybe this is my local Linux build issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment