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 warn modal needs refactoring #2128

Closed
srirambv opened this issue Nov 14, 2018 · 5 comments · Fixed by brave/brave-core#1019
Closed

Sync warn modal needs refactoring #2128

srirambv opened this issue Nov 14, 2018 · 5 comments · Fixed by brave/brave-core#1019

Comments

@srirambv
Copy link
Contributor

srirambv commented Nov 14, 2018

Description

Sync warn modal needs refactoring

Steps to Reproduce

  1. Enable sync on beta with --enable-brave-sync
  2. Click on I have an existing Sync code
  3. Click Setup Sync without entering any code or device name, shows the warn message which is just plain text (see screenshot)

Actual result:

image
image

Expected result:

Should show proper error message which is refactored

Reproduces how often:

Easy

Brave version (brave://version info)

Brave 0.57.6 Chromium: 71.0.3578.31 (Official Build) beta (64-bit)
Revision c88fdf2a4ce19a713615ca4fbde7a0d0b5fe2363-refs/branch-heads/3578@{#427}
OS All

Reproducible on current release:

  • Does it reproduce on brave-browser dev/beta builds?
    Yes on beta build

Website problems only:

  • Does the issue resolve itself when disabling Brave Shields?
  • Is the issue reproducible on the latest version of Chrome?

Additional Information

#2126 also needs a warning similar to this if a device name is mandatory
@rebron @bbondy @kjozwiak added to 1.x backlog as this is just a refactor which isn't priority but if needed please add it to 0.57.x milestone.

@srirambv srirambv added feature/sync priority/P5 Not scheduled. Don't anticipate work on this any time soon. dev-concern labels Nov 14, 2018
@srirambv srirambv added this to the 1.x Backlog milestone Nov 14, 2018
@cezaraugusto
Copy link
Contributor

cc @bradleyrichter @rossmoody

@cezaraugusto cezaraugusto self-assigned this Nov 15, 2018
@kjozwiak
Copy link
Member

Assuming this will be addressed in the sync v2 ui?

@cezaraugusto
Copy link
Contributor

ya correct

@AlexeyBarabash
Copy link
Contributor

Also, it would be good to have this message for

function syncSetupError(error: string) {
    alert('Sync setup error: ' + error)
  }

https://github.com/brave/brave-core/pull/951/files#diff-80431259cfb8c5907f8730d029f18d1eR64

Related issue: #2103

@srirambv
Copy link
Contributor Author

srirambv commented Dec 13, 2018

Verification passed on

Brave 0.58.12 Chromium: 71.0.3578.80 (Official Build) (64-bit)
Revision 2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}
OS Linux

image

Verification passed on

Brave 0.59.8 Chromium: 71.0.3578.98 (Official Build) beta (64-bit)
Revision 15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS Windows

image

Verified passed with

Brave 0.58.12 Chromium: 71.0.3578.80 (Official Build) (64-bit)
Revision 2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}
OS Mac OS X

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

Successfully merging a pull request may close this issue.

7 participants