|
|
DescriptionShow number of hidden items in new-style list notification.
Show number of hidden items on the right bottom of the new-style
notification in both collapsed and expanded state.
BUG=726244
TEST=manual, also tested in RTL language (Hebrew)
Review-Url: https://codereview.chromium.org/2941043004
Cr-Commit-Position: refs/heads/master@{#482198}
Committed: https://chromium.googlesource.com/chromium/src/+/4cf375029b9abf4be32a79fbd7798622e12e569e
Patch Set 1 #Patch Set 2 : Fix number of items shown in the expanded view. #
Total comments: 4
Patch Set 3 : Make overflow indicator localized. #Patch Set 4 : Rebase. #Patch Set 5 : Fix layout problem and some nits. #
Total comments: 4
Patch Set 6 : Resolve review comments. #
Messages
Total messages: 37 (28 generated)
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== WIP: Show number of hidden items in new-style list notification. BUG=726244 TEST=manual ========== to ========== Show number of hidden items in new-style list notification. Show number of hidden items on the right bottom of the new-style notification in both collapsed and expanded state. BUG=726244 TEST=manual ==========
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Description was changed from ========== Show number of hidden items in new-style list notification. Show number of hidden items on the right bottom of the new-style notification in both collapsed and expanded state. BUG=726244 TEST=manual ========== to ========== Show number of hidden items in new-style list notification. Show number of hidden items on the right bottom of the new-style notification in both collapsed and expanded state. BUG=726244 TEST=manual ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tetsui@chromium.org changed reviewers: + fukino@chromium.org, yoshiki@chromium.org
PTAL. Thanks!
Did you check the layout in RTL languages? https://codereview.chromium.org/2941043004/diff/20001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2941043004/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:108: DISALLOW_COPY_AND_ASSIGN(ItemView); please place this at the last. https://codereview.chromium.org/2941043004/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:145: base::IntToString16(num_remaining)); Could you use a localized string instead of just "+" and number? This string can be localized in Android.
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Show number of hidden items in new-style list notification. Show number of hidden items on the right bottom of the new-style notification in both collapsed and expanded state. BUG=726244 TEST=manual ========== to ========== Show number of hidden items in new-style list notification. Show number of hidden items on the right bottom of the new-style notification in both collapsed and expanded state. BUG=726244 TEST=manual, also tested in RTL language (Hebrew) ==========
I moved the indicator to the localized string and also fixed a layout problem. Verified it works properly with Hebrew setting and also verified the behavior is consistent with Android's one in Hebrew. https://codereview.chromium.org/2941043004/diff/20001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2941043004/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:108: DISALLOW_COPY_AND_ASSIGN(ItemView); On 2017/06/21 07:32:12, yoshiki wrote: > please place this at the last. Done. https://codereview.chromium.org/2941043004/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:145: base::IntToString16(num_remaining)); On 2017/06/21 07:32:12, yoshiki wrote: > Could you use a localized string instead of just "+" and number? This string can > be localized in Android. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a nit https://codereview.chromium.org/2941043004/diff/80001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2941043004/diff/80001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:112: class LayoutManager : public views::FillLayout { nit: This class can be private.
https://codereview.chromium.org/2941043004/diff/80001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2941043004/diff/80001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:579: ItemView* item_view = new ItemView(items[i], items.size() - i - 1); Optional nit: Since only the first and the last item need remaining_count, how about adding SetRemainingCount() to ItemView and call the method only for the first and the last item?
lgtm
Thank you! https://codereview.chromium.org/2941043004/diff/80001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2941043004/diff/80001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:112: class LayoutManager : public views::FillLayout { On 2017/06/26 04:00:13, fukino wrote: > nit: This class can be private. Done. https://codereview.chromium.org/2941043004/diff/80001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:579: ItemView* item_view = new ItemView(items[i], items.size() - i - 1); On 2017/06/26 04:18:37, fukino wrote: > Optional nit: Since only the first and the last item need remaining_count, how > about adding SetRemainingCount() to ItemView and call the method only for the > first and the last item? Done.
The CQ bit was checked by tetsui@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org, fukino@chromium.org Link to the patchset: https://codereview.chromium.org/2941043004/#ps100001 (title: "Resolve review comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498453852316370, "parent_rev": "56a7cdb2df5c350fbd1c97d0f35aada0d3d67b9b", "commit_rev": "4cf375029b9abf4be32a79fbd7798622e12e569e"}
Message was sent while issue was closed.
Description was changed from ========== Show number of hidden items in new-style list notification. Show number of hidden items on the right bottom of the new-style notification in both collapsed and expanded state. BUG=726244 TEST=manual, also tested in RTL language (Hebrew) ========== to ========== Show number of hidden items in new-style list notification. Show number of hidden items on the right bottom of the new-style notification in both collapsed and expanded state. BUG=726244 TEST=manual, also tested in RTL language (Hebrew) Review-Url: https://codereview.chromium.org/2941043004 Cr-Commit-Position: refs/heads/master@{#482198} Committed: https://chromium.googlesource.com/chromium/src/+/4cf375029b9abf4be32a79fbd779... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4cf375029b9abf4be32a79fbd779... |