Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
914353004:
Update import details panel to comply (mostly) with UX direction. (Closed)
|
Created:
5 years, 10 months ago by Steve McKay Modified:
5 years, 10 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, tfarina, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate import details panel to comply (mostly) with UX direction.
This is a check-point CL. I'll be following up this change with additional fine-tuning of the UI and some additional tests + the ability to open a new drive window when the destination link is clicked.
TBR=cpu@chromium.org for .grd changes
BUG=420680
TEST=browser_test: FileManagerJsTest.*
Committed: https://crrev.com/b49c0e6f44c59f4a32c53ccf1010f623bdcc4f79
Cr-Commit-Position: refs/heads/master@{#316178}
Patch Set 1 #
Total comments: 48
Patch Set 2 : Respond to review comments. #
Total comments: 6
Patch Set 3 : Send along scan when making a UI update while importing. #Patch Set 4 : Respond to review comments. #Patch Set 5 : CSS tweaks #Patch Set 6 : Merge master. #
Total comments: 3
Patch Set 7 : Exclude details panel button from tab index. #
Total comments: 6
Messages
Total messages: 36 (13 generated)
smckay@chromium.org changed reviewers: + mtomasz@chromium.org
mtomasz@chromium.org changed reviewers: + kenobi@chromium.org
lgtm with nits, but please get a quick check from Ben before committing https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:451: <message name="IDS_FILE_BROWSER_CLOUD_IMPORT_TITLE" desc="Title of the cloud import feature."> nit: feature -> dialog? https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:452: Cloud backup nit: What's the final naming? Back up backup or import? https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:455: ChromeOS Cloud backup nit: ChromeOS -> Chrome OS nit: Cloud backup -> cloud backup? https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:461: Backup your photos and videos. nit: Not period for tooltips? https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:464: Show Cloud backup details. nit: ditto https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:470: <ph name="BEGIN_LINK"><span class='leader'></ph> nit: Do we need the class name in strings? https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:479: <ph name="BEGIN_LINK"><span class='leader'></ph>All backed up!<ph name="END_LINK"></span></ph> nit: Remove span? https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:482: All backed up! nit: Why no media shows "All backed up!"? Could you explain it in desc="..." so people translating have more context? https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:491: Try selecting a few photos to start nit: period missing https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:494: Not enough space on device. nit: No period for tooltips. nit: on device -> on the device? https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:497: <ph name="BEGIN_LINK"><span class='leader'></ph> I think we are putting to much in strings. If we change the UI, we will have to send many strings to translation again. Can we split these strings? Especially the "new photos found" should be a separate string. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:501: Ready to backup to <ph name="BEGIN_LINK"><span class='destination-link'></ph>Google Drive<ph name="END_LINK"></span></ph> nit: Do we need a class name in this string? https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:506: <message name="IDS_FILE_BROWSER_CLOUD_IMPORT_STATUS_IMPORTING" desc="Cloud import active-backup status."> nit: People doing translations may not be famililar with the cloud import UI at all. Can we put more useful context in desc="..."? Eg. "Message shown when importing is in progress"? https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/css/file_manager.css (right): https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:354: font-family: 'Roboto'; nit: Roboto should be default in the entire Files app already, could you confirm? https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:357: top: 38px; Please comment all magical numbers. Does it have to be in sync with eg. the header height or the button position, etc? https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:358: transition: all 0.2s ease; Position transition with top/left/right/bottom are janky. Please use transform: translateX(...) instead. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:370: border-radius: 4px 4px 0px 0px ; nit: \s; -> ; https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:373: font-size: 12pt; nit: Please use percents for font sizes, so the UI scales correctly when different default font size is chosen. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:602: this.detailsPanel_.style.display = 'block'; nit: Can we use this.detailsPanel_.hidden = false/true? https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:607: // When hiding outself, we want to wait for the visually We can listen to webkitTransitionEnd instead. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:630: this.importButton_.style.display = 'none'; nit: hidden = true here and in other places https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:658: this.importButton_.style.display = 'block'; nit: ditto here and in other places https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:758: // this.photoCount_.textContent = 0; nit: Is this commented out on purpose? https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/main.html (right): https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/main.html:358: style='display: none;'> nit: display: none -> hidden attribute?
New patchsets have been uploaded after l-g-t-m from mtomasz@chromium.org
Respond to review comments.
Dones. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:451: <message name="IDS_FILE_BROWSER_CLOUD_IMPORT_TITLE" desc="Title of the cloud import feature."> On 2015/02/12 03:22:01, mtomasz wrote: > nit: feature -> dialog? Used in a couple locations. So it is the general purpose feature title. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:452: Cloud backup On 2015/02/12 03:22:01, mtomasz wrote: > nit: What's the final naming? Back up backup or import? This is the final naming. "Cloud backup", but the call to action is "Back Up". https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:455: ChromeOS Cloud backup On 2015/02/12 03:22:00, mtomasz wrote: > nit: ChromeOS -> Chrome OS > nit: Cloud backup -> cloud backup? That's the title. I dunno why we treat title like this, I don't personally like it, but that's what UX wants. "Chrome OS" + "Cloud backup". https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:461: Backup your photos and videos. On 2015/02/12 03:22:01, mtomasz wrote: > nit: Not period for tooltips? Done. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:464: Show Cloud backup details. On 2015/02/12 03:22:00, mtomasz wrote: > nit: ditto Done. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:470: <ph name="BEGIN_LINK"><span class='leader'></ph> On 2015/02/12 03:22:00, mtomasz wrote: > nit: Do we need the class name in strings? It's better than having to pass it in. To be honest what we need is a real templating system, because this arrangement is a ROYAL pain in the ass. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:479: <ph name="BEGIN_LINK"><span class='leader'></ph>All backed up!<ph name="END_LINK"></span></ph> On 2015/02/12 03:22:01, mtomasz wrote: > nit: Remove span? No, that's the end </span> tag. Again, need a real templating system. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:482: All backed up! On 2015/02/12 03:22:00, mtomasz wrote: > nit: Why no media shows "All backed up!"? Could you explain it in desc="..." so > people translating have more context? Done. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:491: Try selecting a few photos to start On 2015/02/12 03:22:01, mtomasz wrote: > nit: period missing Done. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:494: Not enough space on device. On 2015/02/12 03:22:00, mtomasz wrote: > nit: No period for tooltips. > nit: on device -> on the device? Done. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:497: <ph name="BEGIN_LINK"><span class='leader'></ph> On 2015/02/12 03:22:01, mtomasz wrote: > I think we are putting to much in strings. If we change the UI, we will have to > send many strings to translation again. Can we split these strings? Especially > the "new photos found" should be a separate string. That makes things ever harder to manage from the controller side. Again, what we need is a templating system...where that templating system supports automatic extraction of strings for translation. We just don't have anything in Chrome like that available to us. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:501: Ready to backup to <ph name="BEGIN_LINK"><span class='destination-link'></ph>Google Drive<ph name="END_LINK"></span></ph> On 2015/02/12 03:22:01, mtomasz wrote: > nit: Do we need a class name in this string? Yes. In order to style this. Before I submit, I'm going to try (again) using the ::first-line meta selector...I wasn't getting ideal results when I frist tried. But if that works, I can drop the markup of the first line from the strings...making them MUCH more readable. https://codereview.chromium.org/914353004/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:506: <message name="IDS_FILE_BROWSER_CLOUD_IMPORT_STATUS_IMPORTING" desc="Cloud import active-backup status."> On 2015/02/12 03:22:01, mtomasz wrote: > nit: People doing translations may not be famililar with the cloud import UI at > all. Can we put more useful context in desc="..."? Eg. > "Message shown when importing is in progress"? Done. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/css/file_manager.css (right): https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:354: font-family: 'Roboto'; On 2015/02/12 03:22:01, mtomasz wrote: > nit: Roboto should be default in the entire Files app already, could you > confirm? Done. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:357: top: 38px; On 2015/02/12 03:22:01, mtomasz wrote: > Please comment all magical numbers. Does it have to be in sync with eg. the > header height or the button position, etc? Done. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:358: transition: all 0.2s ease; On 2015/02/12 03:22:01, mtomasz wrote: > Position transition with top/left/right/bottom are janky. Please use transform: > translateX(...) instead. Done. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:370: border-radius: 4px 4px 0px 0px ; On 2015/02/12 03:22:01, mtomasz wrote: > nit: \s; -> ; Done. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/css/file_manager.css:373: font-size: 12pt; On 2015/02/12 03:22:01, mtomasz wrote: > nit: Please use percents for font sizes, so the UI scales correctly when > different default font size is chosen. K. Done. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:602: this.detailsPanel_.style.display = 'block'; On 2015/02/12 03:22:01, mtomasz wrote: > nit: Can we use this.detailsPanel_.hidden = false/true? Done. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:607: // When hiding outself, we want to wait for the visually On 2015/02/12 03:22:01, mtomasz wrote: > We can listen to webkitTransitionEnd instead. Oh, thank you for saving me from that. I felt so dirty using a timeout. I should remember that *everything* in web platform has an event. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:630: this.importButton_.style.display = 'none'; On 2015/02/12 03:22:01, mtomasz wrote: > nit: hidden = true > here and in other places Done. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:658: this.importButton_.style.display = 'block'; On 2015/02/12 03:22:01, mtomasz wrote: > nit: ditto here and in other places Done. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/import_controller.js:758: // this.photoCount_.textContent = 0; On 2015/02/12 03:22:01, mtomasz wrote: > nit: Is this commented out on purpose? Not needed anymore. Gone. https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/main.html (right): https://codereview.chromium.org/914353004/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/main.html:358: style='display: none;'> On 2015/02/12 03:22:01, mtomasz wrote: > nit: display: none -> hidden attribute? Done.
lgtm lgtm with a couple of nits. https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/import_controller.js:178: // this.environment_.setCurrentDirectory(); Remove this? https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/import_controller.js:594: console.assert(!!this.destinationListener_); nit: add an assert message https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/import_controller.js:762: // this.photoCount_.textContent = 0; Remove this?
New patchsets have been uploaded after l-g-t-m from kenobi@chromium.org
Send along scan when making a UI update while importing.
Respond to review comments.
Dones. Thakns! https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/import_controller.js:178: // this.environment_.setCurrentDirectory(); On 2015/02/12 23:31:15, Ben Kwa wrote: > Remove this? Done. https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/import_controller.js:594: console.assert(!!this.destinationListener_); On 2015/02/12 23:31:15, Ben Kwa wrote: > nit: add an assert message Done. https://codereview.chromium.org/914353004/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/import_controller.js:762: // this.photoCount_.textContent = 0; On 2015/02/12 23:31:15, Ben Kwa wrote: > Remove this? Done.
The CQ bit was checked by smckay@chromium.org
The CQ bit was unchecked by smckay@chromium.org
CSS tweaks
The CQ bit was checked by smckay@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914353004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
Merge master.
The CQ bit was checked by smckay@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914353004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by smckay@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914353004/100001
fukino@chromium.org changed reviewers: + fukino@chromium.org
https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_ma... File ui/file_manager/file_manager/main.html (right): https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/main.html:384: <paper-button class="import"> tabindex="-1" for this paper-button will fix the browser tests. After Ctrl+F => Tab, this invisible button has focus and it breaks the test.
https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_ma... File ui/file_manager/file_manager/main.html (right): https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/main.html:350: hidden='true'> hidden> should be sufficient. https://codereview.chromium.org/914353004/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/main.html:358: hidden='true'> ditto
Exclude details panel button from tab index.
The CQ bit was checked by smckay@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914353004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b49c0e6f44c59f4a32c53ccf1010f623bdcc4f79 Cr-Commit-Position: refs/heads/master@{#316178}
Message was sent while issue was closed.
See: crbug.com/458510 https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/import_controller.js:603: this.setDetailsVisible(this.detailsPanel_.className === 'hidden'); Hidden attribute should be used instead? https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/import_controller.js:609: this.detailsPanel_.className = ''; ditto https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/import_controller.js:611: this.detailsPanel_.className = 'hidden'; ditto
Message was sent while issue was closed.
https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/import_controller.js (right): https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/import_controller.js:603: this.setDetailsVisible(this.detailsPanel_.className === 'hidden'); On 2015/02/13 13:26:24, mtomasz wrote: > Hidden attribute should be used instead? We both mark the element as hidden AND add the 'hidden' class to the element. The 'hidden' class provides the smooth and sensual visual transition when showing/hiding. https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/import_controller.js:609: this.detailsPanel_.className = ''; On 2015/02/13 13:26:24, mtomasz wrote: > ditto Acknowledged. https://codereview.chromium.org/914353004/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/import_controller.js:611: this.detailsPanel_.className = 'hidden'; On 2015/02/13 13:26:24, mtomasz wrote: > ditto Acknowledged. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
