New issue
Advanced search Search tips

Issue 659259 link

Starred by 2 users
Status: Fixed
Owner:
dpa...@chromium.org
Closed: Nov 2016
Cc:
mahmadi@chromium.org
dbeam@chromium.org
tbuck...@chromium.org
bettes@chromium.org
dpa...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Blocked on:
issue 666035
Blocking:
issue 614588



Sign in to add a comment

[People] Import Data Dialog

Project Member Reported by tommycli@chromium.org, Oct 25 2016 
Hey, are we planning to implement chrome://settings/importData ?

I don't see it in the mocks or code. It's from the old People section.

Comment 1 by tbuck...@chromium.org, Oct 26 2016

Cc: dbeam@chromium.org bettes@chromium.org mahmadi@chromium.org
Labels: Hotlist-MD-Settings-People OS-All
Owner: tommycli@chromium.org
Status: Assigned (was: Untriaged)
How do users actually trigger this dialog? All the instructions I've found suggest going through chrome://bookmarks (eg. https://support.google.com/chrome/answer/96816?hl=en)

Comment 2 by dbeam@chromium.org, Oct 26 2016

Cc: tbuck...@chromium.org

Comment 3 by tommycli@chromium.org, Nov 3 2016 

Indeed there is also UI surface within Settings to get there:
Screenshot from 2016-11-02 17:34:51.png
29.5 KB View Download

Comment 4 by tommycli@chromium.org, Nov 3 2016 

bettes: I think we need mocks for both the entry point into Import Data, as well as the actual Import Data dialog. (Or subpage)

I didn't notice this was a feature missing from MD Settings until I did the audit of legacy URLs...
Screenshot from 2016-11-02 17:36:15.png
21.2 KB View Download

Comment 5 by tommycli@chromium.org, Nov 3 2016

Blocking: 614588
Marking as a dev blocker.

Comment 6 by dbeam@chromium.org, Nov 3 2016

Labels: -Pri-2 Pri-1
tommycli@: got confirmation that bettes@ and tbuckley@ want to keep this a dialog

specific instructions from bettes@:

> If we can use the same max-height as the cbd (if applicable)
> And 512px width

i think otherwise, just fill in from existing UIs

Comment 7 by tommycli@chromium.org, Nov 3 2016 

Thanks. I'll get started on this.

Comment 11 by dpa...@chromium.org, Nov 10 2016 

FYI, I'll attempt to advance progress in this bug until @tommycli is back, by implementing the dialog UI first. Then someone (me or @tommycli) can hook it up to the existing ImportDataBrowserProxy.

Comment 12 by dpa...@chromium.org, Nov 11 2016 

@bettes: As stated at comment#4, a UX decision is needed on how the user can open this dialog. There should probably be a button similar to the old Options "Import bookmarks and settings" somewhere.

FYI, I have started implementing this dialog.

Comment 13 by dpa...@chromium.org, Nov 11 2016 

See screenshot on how the button could look like.

I do find it a bit suboptimal though that the first thing the user sees when visiting MD Settings is the "People" section where parts of it are not that common (adding/removing users, importing data from other browsers). I guess that ship has already sailed when we decided to merge the "sync" section with the "people" section, unlike old Options.
import_data_button.png
27.7 KB View Download

Comment 14 by bettes@chromium.org, Nov 11 2016 

Use a single line row, with an arrow indicator, positioned as the last row item in the People's section. 

------------------------------------
Import bookmarks and settings      >
------------------------------------
Screen Shot 2016-11-11 at 11.12.08 AM.png
44.4 KB View Download

Comment 15 by dpa...@chromium.org, Nov 11 2016 

Thanks for the quick response!

Comment 16 by dbeam@chromium.org, Nov 14 2016

Owner: dpa...@chromium.org
Status: Started (was: Assigned)
https://codereview.chromium.org/2500653002/

Comment 17 by dpa...@chromium.org, Nov 14 2016 

@tbuckley, @bettes: Any ideas on how should the "import successful" message be displayed to the user? See screencast on how old Options does this.

Asking because the implementation is coming together quickly (https://codereview.chromium.org/2500653002), and I am getting to the point where the UI dialog is hooked up to the C++.
import_success.mp4
107 KB View Download

Comment 18 by bettes@chromium.org, Nov 15 2016 

Cool! A few thoughts: 

1. Can we avoid flashing between 2 different dialogs of import and success? Instead, can the success UI be in the same dialog as import? (We should have a fixed height already)

2. Remove the "Success!" headline string. Instead, do the following:


Import bookmarks and settings      x
____________________________________

              ( ✓ )


Your bookmarks and settings are ready. 

Always show the bookmarks bar     --o
 

                               |Done|

Comment 19 by dbeam@chromium.org, Nov 15 2016 

note: this UI is triggered from an empty bookmarks bar (see attached), so:

1) the URL matters and is opened by native UI (so we should test this flow)
2) that's why we show the bookmarks bar toggle/checkbox (as the bookmarks bar may have just become more useful if you imported a bunch of bookmarks)
Screen Shot 2016-11-15 at 10.22.49 AM.png
51.4 KB View Download

Comment 20 by dpa...@chromium.org, Nov 15 2016 

@bettes: Does this match your expectations from comment #18 (see screenshot)?

Regarding the height of the dialog, see screencast.

I have implemented as one dialog, but the contents are still determining the height. If I add a forced height, then the "success" case ends up having a lot of white-space. Adjusting the size of the dialog based on currently displayed contents seems simpler and better to me, WDYT?
import_success.png
16.0 KB View Download
import_success_new.mp4
378 KB View Download
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2a979f3c638e45d6481c68d0cea907d63bb61cb

commit c2a979f3c638e45d6481c68d0cea907d63bb61cb
Author: dpapad <dpapad@chromium.org>
Date: Tue Nov 15 22:15:09 2016

MD Settings: Add import data dialog.

BUG= 659259 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2500653002
Cr-Commit-Position: refs/heads/master@{#432268}

[modify] https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb/chrome/app/settings_strings.grdp
[modify] https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb/chrome/browser/extensions/api/settings_private/prefs_util.cc
[modify] https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb/chrome/browser/resources/settings/people_page/compiled_resources2.gyp
[add] https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb/chrome/browser/resources/settings/people_page/import_data_dialog.html
[add] https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb/chrome/browser/resources/settings/people_page/import_data_dialog.js
[modify] https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb/chrome/browser/resources/settings/people_page/people_page.html
[modify] https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb/chrome/browser/resources/settings/people_page/people_page.js
[modify] https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/c2a979f3c638e45d6481c68d0cea907d63bb61cb/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

Comment 22 by dpa...@chromium.org, Nov 16 2016

Blockedon: 666035
Project Member

Comment 23 by bugdroid1@chromium.org, Nov 17 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/411c9f114fd32cde3b27382c78fdaac5d4e0365f

commit 411c9f114fd32cde3b27382c78fdaac5d4e0365f
Author: dpapad <dpapad@chromium.org>
Date: Thu Nov 17 05:06:01 2016

MD Settings: Hook up import data dialog.

BUG= 659259 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2501783003
Cr-Commit-Position: refs/heads/master@{#432753}

[modify] https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f/chrome/app/settings_chromium_strings.grdp
[modify] https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f/chrome/app/settings_google_chrome_strings.grdp
[modify] https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f/chrome/app/settings_strings.grdp
[modify] https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f/chrome/browser/resources/settings/people_page/compiled_resources2.gyp
[modify] https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f/chrome/browser/resources/settings/people_page/import_data_dialog.html
[modify] https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f/chrome/browser/resources/settings/people_page/import_data_dialog.js
[modify] https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
[modify] https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f/chrome/browser/ui/webui/settings/settings_import_data_handler.cc

Comment 24 by dpa...@chromium.org, Nov 17 2016

Status: Fixed (was: Started)
►

Sign in to add a comment