Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2120283003:
NTP, modify the snippet header display to above the peeking card (Closed)
|
Created:
4 years, 5 months ago by mcwilliams Modified:
4 years, 5 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNTP, modify the snippet header display to above the peeking card
Currently the NTP header displays when the first card reaches the top of the screen, this was good when we have snap scrolling which has now been removed. The heading height will now increase and show from under the peeking card at a percent of the scroll when the card is peeking, the card will peek at the balance of the scroll value until the header is at max height and normal scroll will resume.
videos: https://drive.google.com/open?id=0B1IgAIJ9cgizY3AwUGJfRC1RVWc
BUG=624439
Committed: https://crrev.com/8aeb3bf3e1304d72bc1ea65942d216555ad3dc13
Cr-Commit-Position: refs/heads/master@{#403802}
Patch Set 1 #Patch Set 2 : Refine snap scrolling on the Cards New Tab Page. #
Total comments: 22
Patch Set 3 : Refine snap scrolling on the Cards New Tab Page. #Patch Set 4 : Refine snap scrolling on the Cards New Tab Page. #
Total comments: 2
Patch Set 5 : Changes #
Messages
Total messages: 18 (4 generated)
mcwilliams@chromium.org changed reviewers: + bauerb@chromium.org, dgn@chromium.org
https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:309: // screen. Remove the headerView height which gets dynamically increased with scrolling. Nit: If you are referring to the variable, put it between pipe symbols (|headerView|), otherwise use prose (header view). https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:315: + parentScrollY - headerView.getHeight() // B. Nit: I would make this an additional lettered item in the calculation. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:25: private NewTabPageRecyclerView mRecyclerView; Can this be final? https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:62: int amountScrolled = (recyclerViewHeight - mMaxPeekPadding) - snippetHeaderTop; You could extract this to a local variable and then check in the previous line whether it's positive. Or just directly use MathUtils.clamp(amountScrolled * 0.7, 0, mMaxSnippetHeaderHeight)? https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:64: Math.min((int) Math.round(amountScrolled * 0.7), mMaxSnippetHeaderHeight); Can you extract the 0.7 to a constant?
https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:317: + peekingHeight; // D. // the amount of card showing nit: move the second comment to the variable declaration. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:67: if (mHeader.isVisible()) itemView.setAlpha((float) headerHeight / mMaxSnippetHeaderHeight); If height is 0 the header won't be visible for sure, but it also makes sense for Alpha to be 0 in that case. I'd skip the isVisible() check. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:69: itemView.setLayoutParams(params); you don't need to call setLayoutParams again. The reference you get in getLayoutParams at line 53 is directly the one still used on the view, so just setting params.height will update the view.
PTAL https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:309: // screen. Remove the headerView height which gets dynamically increased with scrolling. On 2016/07/05 09:02:09, Bernhard Bauer wrote: > Nit: If you are referring to the variable, put it between pipe symbols > (|headerView|), otherwise use prose (header view). Done. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:315: + parentScrollY - headerView.getHeight() // B. On 2016/07/05 09:02:09, Bernhard Bauer wrote: > Nit: I would make this an additional lettered item in the calculation. Done. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:317: + peekingHeight; // D. // the amount of card showing On 2016/07/05 10:38:19, dgn wrote: > nit: move the second comment to the variable declaration. oops, was a comment for me - removed https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:25: private NewTabPageRecyclerView mRecyclerView; On 2016/07/05 09:02:09, Bernhard Bauer wrote: > Can this be final? Done. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:62: int amountScrolled = (recyclerViewHeight - mMaxPeekPadding) - snippetHeaderTop; On 2016/07/05 09:02:09, Bernhard Bauer wrote: > You could extract this to a local variable and then check in the previous line > whether it's positive. Or just directly use MathUtils.clamp(amountScrolled * > 0.7, 0, mMaxSnippetHeaderHeight)? Done. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:64: Math.min((int) Math.round(amountScrolled * 0.7), mMaxSnippetHeaderHeight); On 2016/07/05 09:02:09, Bernhard Bauer wrote: > Can you extract the 0.7 to a constant? Done. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:67: if (mHeader.isVisible()) itemView.setAlpha((float) headerHeight / mMaxSnippetHeaderHeight); On 2016/07/05 10:38:19, dgn wrote: > If height is 0 the header won't be visible for sure, but it also makes sense for > Alpha to be 0 in that case. I'd skip the isVisible() check. Done. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:69: itemView.setLayoutParams(params); On 2016/07/05 10:38:19, dgn wrote: > you don't need to call setLayoutParams again. The reference you get in > getLayoutParams at line 53 is directly the one still used on the view, so just > setting params.height will update the view. Removing this does not leave desirable behaviour. The peeking card starts disappearing when over scrolling
https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:62: int amountScrolled = (recyclerViewHeight - mMaxPeekPadding) - snippetHeaderTop; On 2016/07/05 13:23:23, mcwilliams wrote: > On 2016/07/05 09:02:09, Bernhard Bauer wrote: > > You could extract this to a local variable and then check in the previous line > > whether it's positive. Or just directly use MathUtils.clamp(amountScrolled * > > 0.7, 0, mMaxSnippetHeaderHeight)? > > Done. I think Bernhard meant int amountScrolled = ... if (mHeader.isVisible() && amountScrolled > 0) { headerHeight = MathUtils.clamp(...) } That being said I'm not sure clamp helps here, since it doesn't handle double->int. It's not helping with the rounding, and a straight int cast rounds down instead of to the nearest value (AFAIK). round() into min() looks good to me :/ https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:69: itemView.setLayoutParams(params); On 2016/07/05 13:23:23, mcwilliams wrote: > On 2016/07/05 10:38:19, dgn wrote: > > you don't need to call setLayoutParams again. The reference you get in > > getLayoutParams at line 53 is directly the one still used on the view, so just > > setting params.height will update the view. > > Removing this does not leave desirable behaviour. The peeking card starts > disappearing when over scrolling Ok my bad. setLayoutParams additionally notifies the view of changes and forces a refresh. It's probably needed because the view is already rendered.
https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:62: int amountScrolled = (recyclerViewHeight - mMaxPeekPadding) - snippetHeaderTop; On 2016/07/05 13:51:17, dgn wrote: > On 2016/07/05 13:23:23, mcwilliams wrote: > > On 2016/07/05 09:02:09, Bernhard Bauer wrote: > > > You could extract this to a local variable and then check in the previous > line > > > whether it's positive. Or just directly use MathUtils.clamp(amountScrolled * > > > 0.7, 0, mMaxSnippetHeaderHeight)? > > > > Done. > > I think Bernhard meant > > int amountScrolled = ... > if (mHeader.isVisible() && amountScrolled > 0) { > headerHeight = MathUtils.clamp(...) > } > > That being said I'm not sure clamp helps here, since it doesn't handle > double->int. It's not helping with the rounding, and a straight int cast rounds > down instead of to the nearest value (AFAIK). round() into min() looks good to > me :/ What I meant was: int amount_scrolled = recyclerViewHeight - mMaxPeekPadding - snippetHeaderTop; if (mHeader.isVisible()) { headerHeight = MathUtils.clamp((int) Math.round(amountScrolled * SCROLL_HEADER_HEIGHT_PERCENTAGE), 0, mMaxSnippetHeaderHeight); } The clamp was to get rid of the `amountScrolled > 0` check, so we'd only have to check for header visibility. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:69: itemView.setLayoutParams(params); On 2016/07/05 13:51:17, dgn wrote: > On 2016/07/05 13:23:23, mcwilliams wrote: > > On 2016/07/05 10:38:19, dgn wrote: > > > you don't need to call setLayoutParams again. The reference you get in > > > getLayoutParams at line 53 is directly the one still used on the view, so > just > > > setting params.height will update the view. > > > > Removing this does not leave desirable behaviour. The peeking card starts > > disappearing when over scrolling > > Ok my bad. setLayoutParams additionally notifies the view of changes and forces > a refresh. It's probably needed because the view is already rendered. You could call itemView.requestLayout() if you've changed the layout param values.
PTAL https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:62: int amountScrolled = (recyclerViewHeight - mMaxPeekPadding) - snippetHeaderTop; On 2016/07/05 14:11:21, Bernhard Bauer wrote: > On 2016/07/05 13:51:17, dgn wrote: > > On 2016/07/05 13:23:23, mcwilliams wrote: > > > On 2016/07/05 09:02:09, Bernhard Bauer wrote: > > > > You could extract this to a local variable and then check in the previous > > line > > > > whether it's positive. Or just directly use MathUtils.clamp(amountScrolled > * > > > > 0.7, 0, mMaxSnippetHeaderHeight)? > > > > > > Done. > > > > I think Bernhard meant > > > > int amountScrolled = ... > > if (mHeader.isVisible() && amountScrolled > 0) { > > headerHeight = MathUtils.clamp(...) > > } > > > > That being said I'm not sure clamp helps here, since it doesn't handle > > double->int. It's not helping with the rounding, and a straight int cast > rounds > > down instead of to the nearest value (AFAIK). round() into min() looks good to > > me :/ > > What I meant was: > > int amount_scrolled = recyclerViewHeight - mMaxPeekPadding - snippetHeaderTop; > if (mHeader.isVisible()) { > headerHeight = MathUtils.clamp((int) Math.round(amountScrolled * > SCROLL_HEADER_HEIGHT_PERCENTAGE), 0, mMaxSnippetHeaderHeight); > } > > The clamp was to get rid of the `amountScrolled > 0` check, so we'd only have to > check for header visibility. Done. https://codereview.chromium.org/2120283003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:69: itemView.setLayoutParams(params); On 2016/07/05 14:11:21, Bernhard Bauer wrote: > On 2016/07/05 13:51:17, dgn wrote: > > On 2016/07/05 13:23:23, mcwilliams wrote: > > > On 2016/07/05 10:38:19, dgn wrote: > > > > you don't need to call setLayoutParams again. The reference you get in > > > > getLayoutParams at line 53 is directly the one still used on the view, so > > just > > > > setting params.height will update the view. > > > > > > Removing this does not leave desirable behaviour. The peeking card starts > > > disappearing when over scrolling > > > > Ok my bad. setLayoutParams additionally notifies the view of changes and > forces > > a refresh. It's probably needed because the view is already rendered. > > You could call itemView.requestLayout() if you've changed the layout param > values. Worked perfectly thanks
lgtm https://codereview.chromium.org/2120283003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2120283003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:73: //itemView.setLayoutParams(params); Remove please :)
non owner lgtm
The CQ bit was checked by mcwilliams@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2120283003/#ps80001 (title: "Changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2120283003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2120283003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:73: //itemView.setLayoutParams(params); On 2016/07/05 15:31:27, Bernhard Bauer wrote: > Remove please :) Done.
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== NTP, modify the snippet header display to above the peeking card Currently the NTP header displays when the first card reaches the top of the screen, this was good when we have snap scrolling which has now been removed. The heading height will now increase and show from under the peeking card at a percent of the scroll when the card is peeking, the card will peek at the balance of the scroll value until the header is at max height and normal scroll will resume. videos: https://drive.google.com/open?id=0B1IgAIJ9cgizY3AwUGJfRC1RVWc BUG=624439 ========== to ========== NTP, modify the snippet header display to above the peeking card Currently the NTP header displays when the first card reaches the top of the screen, this was good when we have snap scrolling which has now been removed. The heading height will now increase and show from under the peeking card at a percent of the scroll when the card is peeking, the card will peek at the balance of the scroll value until the header is at max height and normal scroll will resume. videos: https://drive.google.com/open?id=0B1IgAIJ9cgizY3AwUGJfRC1RVWc BUG=624439 Committed: https://crrev.com/8aeb3bf3e1304d72bc1ea65942d216555ad3dc13 Cr-Commit-Position: refs/heads/master@{#403802} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8aeb3bf3e1304d72bc1ea65942d216555ad3dc13 Cr-Commit-Position: refs/heads/master@{#403802} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
