Add distillation info to Reading List view |
||||||||||||||||||||||||||
Issue descriptionReading List view controller should show the distillation date and the distillation size for distilled entries.
,
Feb 22 2017
,
Feb 22 2017
,
Feb 22 2017
noyau@ suggested to have relative date (1 minute ago, 1 day ago...) instead of absolute date.
,
Feb 23 2017
Hmm. I personally prefer the absolute date. It's cleaner.
,
Feb 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5ee668e963c93af5171f8a2e53052412c8b6a98a commit 5ee668e963c93af5171f8a2e53052412c8b6a98a Author: olivierrobin <olivierrobin@chromium.org> Date: Thu Feb 23 16:07:51 2017 [Reading List iOS] Store distillation date and size. Store distillation size and date in DB. They will be displayed on the Reading List View later. BUG= 694451 Review-Url: https://codereview.chromium.org/2707043002 Cr-Commit-Position: refs/heads/master@{#452508} [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/components/reading_list/ios/proto/reading_list.proto [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/components/reading_list/ios/reading_list_entry.cc [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/components/reading_list/ios/reading_list_entry.h [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/components/reading_list/ios/reading_list_entry_unittest.cc [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/components/reading_list/ios/reading_list_model.h [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/components/reading_list/ios/reading_list_model_impl.cc [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/components/reading_list/ios/reading_list_model_impl.h [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/components/reading_list/ios/reading_list_model_unittest.mm [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/ios/chrome/browser/reading_list/reading_list_download_service.cc [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/ios/chrome/browser/reading_list/reading_list_download_service.h [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/ios/chrome/browser/reading_list/url_downloader.cc [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/ios/chrome/browser/reading_list/url_downloader.h [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/ios/chrome/browser/reading_list/url_downloader_unittest.mm [modify] https://crrev.com/5ee668e963c93af5171f8a2e53052412c8b6a98a/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller_unittest.mm
,
Mar 2 2017
Hi amy Do you have any mocks?
,
Mar 2 2017
Mocks & specs attached for default and edit states. Suggested categories of time delineation: x min ago x hours ago x days ago x months ago x years ago
,
Mar 3 2017
,
Mar 3 2017
Does this validate relative time? Should x min ago be updated in real time when it is displayed? I don't see this format anywhere in Chrome. So how is the rounding done? 2 months ago is 1-2 month ago or 2-3 month ago? Sorry for the naive questions...
,
Mar 5 2017
+ Eric Thanks, Amy. As I said in comment number # 7, I prefer the absolute date because it is more straightforward and less ambiguous. Olivier: to answer your questions, assuming relative time is what we're going with: > Should x min ago be updated in real time when it is displayed? No. It's fine to not update in real time. Starting at this screen for a prolonged period of time is not really a usecase we should prioritize. > I don't see this format anywhere in Chrome. So how is the rounding done? > 2 months ago is 1-2 month ago or 2-3 month ago? 2 months ago exactly would be 1-2 months ago. More than 2 would be 2-3 months ago. i.e. it's 1-2 , 2 inclusive.
,
Mar 14 2017
I found ui/base/l10n/time_format.h that seems to do that. I will test.
,
Mar 16 2017
What should we do for not distilled entries? Should we have a "Not distilled" string?
,
Mar 16 2017
Documenting my offline conversation with Olivier for posterity: - We should not have a "not distilled" string. The lack of the green checkmark is enough to indicate this is not distilled - All cells should have equal height
,
Mar 16 2017
Here is a screenshot of a possible layout respecting #16. Note that the title and URL don't move when detail are displayed, so there is no movement when an article is marked as distilled when the view is displayed.
,
Mar 16 2017
For me the cell feels unbalanced when the info are displayed: the whitespace on top is much larger. Maybe we should center the text displayed at all time (so when you add the information you update the text's y-position).
,
Mar 16 2017
This would require more spacing specs. amyroberts, WDYT?
,
Mar 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c7e51754c49c1cb4f24dacbd694bc15d29ed0e2a commit c7e51754c49c1cb4f24dacbd694bc15d29ed0e2a Author: olivierrobin <olivierrobin@chromium.org> Date: Fri Mar 17 09:35:43 2017 Implement (FORMAT_ELAPSED, LENGTH_LONG) in time_format Use it in https://codereview.chromium.org/2751833005/ BUG= 694451 Review-Url: https://codereview.chromium.org/2747253004 Cr-Commit-Position: refs/heads/master@{#457725} [modify] https://crrev.com/c7e51754c49c1cb4f24dacbd694bc15d29ed0e2a/ui/base/l10n/formatter.cc [modify] https://crrev.com/c7e51754c49c1cb4f24dacbd694bc15d29ed0e2a/ui/base/l10n/time_format.h [modify] https://crrev.com/c7e51754c49c1cb4f24dacbd694bc15d29ed0e2a/ui/base/l10n/time_format_unittest.cc [modify] https://crrev.com/c7e51754c49c1cb4f24dacbd694bc15d29ed0e2a/ui/strings/ui_strings.grd
,
Mar 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ac7ceb17518903b0072836ab722384bad87e24e commit 0ac7ceb17518903b0072836ab722384bad87e24e Author: olivierrobin <olivierrobin@chromium.org> Date: Wed Mar 29 16:54:18 2017 Add Distillation info to Reading List view. If page is distilled, show - Date of distillation in relative format (1 hour ago, 2 days ago). - The size of distillation files in human readable format. BUG= 694451 Review-Url: https://codereview.chromium.org/2751833005 Cr-Commit-Position: refs/heads/master@{#460426} [modify] https://crrev.com/0ac7ceb17518903b0072836ab722384bad87e24e/components/reading_list/core/reading_list_entry.cc [modify] https://crrev.com/0ac7ceb17518903b0072836ab722384bad87e24e/ios/chrome/app/strings/ios_strings.grd [modify] https://crrev.com/0ac7ceb17518903b0072836ab722384bad87e24e/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm [modify] https://crrev.com/0ac7ceb17518903b0072836ab722384bad87e24e/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller_unittest.mm [modify] https://crrev.com/0ac7ceb17518903b0072836ab722384bad87e24e/ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h [modify] https://crrev.com/0ac7ceb17518903b0072836ab722384bad87e24e/ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm [modify] https://crrev.com/0ac7ceb17518903b0072836ab722384bad87e24e/ios/chrome/browser/ui/reading_list/reading_list_coordinator.mm
,
Apr 3 2017
,
Apr 4 2017
Verified on iPad Pro iSO 10.1.1 M59.0.3062.0 Dev Distillation info (Relative time and Size) is displayed for each of the entries.
,
Apr 4 2017
Reopening as Months ago info is not displayed. Hours ago, Days ago info is seen, but Month ago/Months ago is not displayed. Always showing the days count. Screenshots attached: 64 days: https://drive.google.com/file/d/0B-xmXLQhjeKuZGlhaGdfVndjUnc/view 88 days: https://drive.google.com/file/d/0B-xmXLQhjeKuODBJME5CQWZJY1U/view
,
Apr 5 2017
,
Apr 25 2017
Removing RBS. Reassigning.
,
Apr 27 2017
,
Apr 27 2017
This bug requires manual review: There is .grd file changes and we are only 39 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 27 2017
+cmasso for cherry-pick approval. The cherry-pick change is only about the month/months ago display.
,
Apr 28 2017
Please mark the issue as fixed. srikanthg@ could you also verify this on your end on M60?
,
Apr 28 2017
,
Apr 28 2017
,
May 2 2017
Verified on iPhone 6 plus iOS 10.3.1 M60.0.3087.0 Canary months ago info is displayed. Note: The weeks ago info is not displayed instead a days ago info is appearing. Please find the attached screenshot and confirm this is the expected behavior or not. Link for screenshot: https://drive.google.com/a/google.com/file/d/0B8Cek8RsDbF8ZzRwWWpwMXowZm8/view?usp=sharing
,
May 2 2017
Yes, weeks are not displayed on purpose.
,
May 3 2017
,
May 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/050c89a59b81cfd885b3f13054033a39b8cb0428 commit 050c89a59b81cfd885b3f13054033a39b8cb0428 Author: gambard <gambard@chromium.org> Date: Wed May 03 10:20:03 2017 Add month and year to time_format. Add the possibility to have month and years in time format. A year is defined as 365 days. A month is defined as 1year / 12. '1 month' means more than one month and less than two month (same for years). CL mainly made by olivierrobin BUG= 694451 Review-Url: https://codereview.chromium.org/2845693002 Cr-Commit-Position: refs/heads/master@{#467625} (cherry picked from commit bef90b8dea482e8a4801f9c9c0543ab35ef2fd9f) Review-Url: https://codereview.chromium.org/2856143002 . Cr-Commit-Position: refs/branch-heads/3071@{#371} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [modify] https://crrev.com/050c89a59b81cfd885b3f13054033a39b8cb0428/ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm [modify] https://crrev.com/050c89a59b81cfd885b3f13054033a39b8cb0428/ui/base/l10n/formatter.cc [modify] https://crrev.com/050c89a59b81cfd885b3f13054033a39b8cb0428/ui/base/l10n/formatter.h [modify] https://crrev.com/050c89a59b81cfd885b3f13054033a39b8cb0428/ui/base/l10n/time_format.cc [modify] https://crrev.com/050c89a59b81cfd885b3f13054033a39b8cb0428/ui/base/l10n/time_format.h [modify] https://crrev.com/050c89a59b81cfd885b3f13054033a39b8cb0428/ui/base/l10n/time_format_unittest.cc [modify] https://crrev.com/050c89a59b81cfd885b3f13054033a39b8cb0428/ui/strings/ui_strings.grd
,
May 10 2017
Verified in 59.0.3071.50 beta, iPhone 6 plus iOS 10.2.1 Strings that are showing are: Just now, 4 minutes ago, 18 days ago, 1 month ago... Distillation size is also shown. Looks good. |
||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||
Comment 1 by olivierrobin@chromium.org
, Feb 21 2017