Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1995433003:
Adding elevation to NTP snippets (Closed)
|
Created:
4 years, 7 months ago by mcwilliams Modified:
4 years, 7 months ago CC:
chromium-reviews, mvanouwerkerk+watch_chromium.org, mcwilliams+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding elevation to NTP snippets
Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen
Video: https://drive.google.com/open?id=0B1IgAIJ9cgizb2FWM29QU1JSSU0
BUG=611974, 611445
Committed: https://crrev.com/84ee518a8941b23498f9c3dc6c5421808143c855
Cr-Commit-Position: refs/heads/master@{#394563}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 33 (15 generated)
Description was changed from ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen BUG=611974 ========== to ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen Video: https://drive.google.com/open?id=0B1IgAIJ9cgizaDNuNzJPU0JUQms BUG=611974 ==========
mcwilliams@chromium.org changed reviewers: + bauerb@chromium.org
Description was changed from ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen Video: https://drive.google.com/open?id=0B1IgAIJ9cgizaDNuNzJPU0JUQms BUG=611974 ========== to ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen Video: https://drive.google.com/open?id=0B1IgAIJ9cgizb2FWM29QU1JSSU0 BUG=611974 ==========
The CQ bit was checked by mcwilliams@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995433003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995433003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1995433003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1995433003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:31: if (childPos == adapter.getItemCount() - 1 || childPos == RecyclerView.NO_POSITION) { The first condition here made sense in the previous version where the only thing we used this decoration for was to add space between items. Now it's technically not incorrect, but only because the last item is never going to be a snippet, and it feels a bit suboptimal to depend on this, in particular if it happens so hidden here. What I would suggest is moving the condition to line 46, or maybe even making it an assert there (so we are explicit about the assumption we're making). https://codereview.chromium.org/1995433003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:35: boolean isViewTypeSnippet = Given that the class name suggests that this decoration applies to snippets, would it make sense to just return early if the item is not a snippet? https://codereview.chromium.org/1995433003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:40: view.setElevation(parent.getContext().getResources().getDimensionPixelSize( You should annotate this method with @TargetApi(Build.VERSION_CODES.LOLLIPOP), otherwise you'll get a lint warning. Alternatively, we could move this into a method on ApiCompatibilityUtils that applies elevation only if supported?
Will the new logic also fix the separator glitches? https://bugs.chromium.org/p/chromium/issues/detail?id=611445 when swiping away snippets?
torne@chromium.org changed reviewers: + torne@chromium.org
base/android LGTM
On 2016/05/18 16:21:26, dgn wrote: > Will the new logic also fix the separator glitches? > https://bugs.chromium.org/p/chromium/issues/detail?id=611445 when swiping away > snippets? Yes, did not know there was a bug for this :)
Description was changed from ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen Video: https://drive.google.com/open?id=0B1IgAIJ9cgizb2FWM29QU1JSSU0 BUG=611974 ========== to ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen Video: https://drive.google.com/open?id=0B1IgAIJ9cgizb2FWM29QU1JSSU0 BUG=611974, 611445 ==========
PTAL https://codereview.chromium.org/1995433003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1995433003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:31: if (childPos == adapter.getItemCount() - 1 || childPos == RecyclerView.NO_POSITION) { On 2016/05/18 14:49:38, Bernhard Bauer wrote: > The first condition here made sense in the previous version where the only thing > we used this decoration for was to add space between items. Now it's technically > not incorrect, but only because the last item is never going to be a snippet, > and it feels a bit suboptimal to depend on this, in particular if it happens so > hidden here. What I would suggest is moving the condition to line 46, or maybe > even making it an assert there (so we are explicit about the assumption we're > making). Done. https://codereview.chromium.org/1995433003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:35: boolean isViewTypeSnippet = On 2016/05/18 14:49:38, Bernhard Bauer wrote: > Given that the class name suggests that this decoration applies to snippets, > would it make sense to just return early if the item is not a snippet? Done. https://codereview.chromium.org/1995433003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:40: view.setElevation(parent.getContext().getResources().getDimensionPixelSize( On 2016/05/18 14:49:38, Bernhard Bauer wrote: > You should annotate this method with @TargetApi(Build.VERSION_CODES.LOLLIPOP), > otherwise you'll get a lint warning. Alternatively, we could move this into a > method on ApiCompatibilityUtils that applies elevation only if supported? Done.
The CQ bit was checked by mcwilliams@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995433003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995433003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
The CQ bit was checked by mcwilliams@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995433003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995433003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
Drive-by review :-) https://codereview.chromium.org/1995433003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1995433003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:19: * A decorator that places separators between RecyclerView items of type nit: this should be about elevation now rather than separators.
https://codereview.chromium.org/1995433003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java (right): https://codereview.chromium.org/1995433003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:19: * A decorator that places separators between RecyclerView items of type On 2016/05/18 17:28:25, Michael van Ouwerkerk wrote: > nit: this should be about elevation now rather than separators. It technically does both - added the elevation section
lgtm
The CQ bit was checked by mcwilliams@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/1995433003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995433003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995433003/80001
Message was sent while issue was closed.
Description was changed from ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen Video: https://drive.google.com/open?id=0B1IgAIJ9cgizb2FWM29QU1JSSU0 BUG=611974, 611445 ========== to ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen Video: https://drive.google.com/open?id=0B1IgAIJ9cgizb2FWM29QU1JSSU0 BUG=611974, 611445 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen Video: https://drive.google.com/open?id=0B1IgAIJ9cgizb2FWM29QU1JSSU0 BUG=611974, 611445 ========== to ========== Adding elevation to NTP snippets Add elevation to the snippet cards so the peeking card is more prominent and the last snippet has elevation especially when you dismiss cards and when there is a space between the last snippet and the bottom of the screen Video: https://drive.google.com/open?id=0B1IgAIJ9cgizb2FWM29QU1JSSU0 BUG=611974, 611445 Committed: https://crrev.com/84ee518a8941b23498f9c3dc6c5421808143c855 Cr-Commit-Position: refs/heads/master@{#394563} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/84ee518a8941b23498f9c3dc6c5421808143c855 Cr-Commit-Position: refs/heads/master@{#394563} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
