Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1944783002:
Add a third point to snap the scroll to. (Closed)
DescriptionAdd a third point to snap the scroll to.
When the Most Visited sights is not fully visible when scrolled to the top, add it as a snap scroll point.
BUG=609070
Committed: https://crrev.com/10cd79b95d90af979f1cea61b6ebd27a275a06a0
Cr-Commit-Position: refs/heads/master@{#391777}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Messages
Total messages: 23 (8 generated)
peconn@chromium.org changed reviewers: + maybelle@chromium.org, mcwilliams@chromium.org
The logic for snap scrolling is pretty flexible - I've got this scrolling to all the correct points, I just need to figure out the correct time to scroll to those points. Please add comments!
Revised version, more comments!
peconn@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/1944783002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1944783002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:468: mRecyclerView.getHeight() < mMostVisitedLayout.getBottom(); Is it possible to have a case where the screen is small enough that there's no MV at all at the start?
https://codereview.chromium.org/1944783002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1944783002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:468: mRecyclerView.getHeight() < mMostVisitedLayout.getBottom(); On 2016/05/04 15:45:19, May wrote: > Is it possible to have a case where the screen is small enough that there's no > MV at all at the start? The Most Visited is contained within the mContentView which is the first item in the RecyclerView (which we check is visible on line 451), so mMostVisited will exist.
lgtm https://codereview.chromium.org/1944783002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1944783002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:464: // If Most Likely is fully visible when we are scrolled to the top, we have two If Most Likely is NOT :) visibile
Diagram showing scrolling behaviour: https://docs.google.com/a/google.com/drawings/d/154ejc3ln4mH7u-dv3CobZ82Pbc84...
https://codereview.chromium.org/1944783002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1944783002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:464: // If Most Likely is fully visible when we are scrolled to the top, we have two On 2016/05/04 16:07:16, mcwilliams wrote: > If Most Likely is NOT :) visibile Nope, I think you misread the comment...
The CQ bit was checked by peconn@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/1944783002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1944783002/20001
LGTM If we end up doing more snap scrolling, I think it would make sense to extract the general logic here into something that can be reused and where we only have to pass a list of snap points.
Description was changed from ========== Add a third point to snap the scroll to. BUG=tba ========== to ========== Add a third point to snap the scroll to. When the Most Visited sights is not fully visible when scrolled to the top, add it as a snap scroll point. BUG=609070 ==========
lgtm Spoke offline with Peter -- behavior is undefined on small screens if we don't have ML items at all, but realistically if we have no ML (assuming due to network issues) we'd likely not have snippets either, so snap points may not be an issue.
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 peconn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1944783002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1944783002/20001
Message was sent while issue was closed.
Description was changed from ========== Add a third point to snap the scroll to. When the Most Visited sights is not fully visible when scrolled to the top, add it as a snap scroll point. BUG=609070 ========== to ========== Add a third point to snap the scroll to. When the Most Visited sights is not fully visible when scrolled to the top, add it as a snap scroll point. BUG=609070 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add a third point to snap the scroll to. When the Most Visited sights is not fully visible when scrolled to the top, add it as a snap scroll point. BUG=609070 ========== to ========== Add a third point to snap the scroll to. When the Most Visited sights is not fully visible when scrolled to the top, add it as a snap scroll point. BUG=609070 Committed: https://crrev.com/10cd79b95d90af979f1cea61b6ebd27a275a06a0 Cr-Commit-Position: refs/heads/master@{#391777} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/10cd79b95d90af979f1cea61b6ebd27a275a06a0 Cr-Commit-Position: refs/heads/master@{#391777} |
