Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2144413003:
NTP, peeking card and heading hiding constraints (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, peeking card and heading hiding constraints
Only allow the card to peek and heading to hide when the above the fold section is fully rendered.
video: https://drive.google.com/corp/drive/folders/0B1IgAIJ9cgizR29mUHN3WHJ4U2s
BUG=626657
Committed: https://crrev.com/cff297f895ebef1cbee11cecc5ab20183582d0d1
Cr-Commit-Position: refs/heads/master@{#406320}
Patch Set 1 #Patch Set 2 : changes #Patch Set 3 : changes #
Total comments: 19
Patch Set 4 : changes #
Total comments: 6
Patch Set 5 : Changes #Patch Set 6 : Changes #Patch Set 7 : Changes #
Total comments: 8
Patch Set 8 : changes #
Total comments: 2
Patch Set 9 : changes #
Messages
Total messages: 31 (10 generated)
Description was changed from ========== NTP, peeking card and heading hiding constraints Only allow the card to peek and heading to hide when the above the fold section is fully rendered. BUG=626657 ========== to ========== NTP, peeking card and heading hiding constraints Only allow the card to peek and heading to hide when the above the fold section is fully rendered. video: https://drive.google.com/corp/drive/folders/0B1IgAIJ9cgizR29mUHN3WHJ4U2s BUG=626657 ==========
mcwilliams@chromium.org changed reviewers: + bauerb@chromium.org
peconn@chromium.org changed reviewers: + peconn@chromium.org
https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:142: return mRecyclerView.computeVerticalScrollOffset() <= itemView.getHeight(); Can't we use functions from the RecyclerView's LayoutManager (http://stackoverflow.com/a/25053500) ?
The CQ bit was checked by mvanouwerkerk@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...
just some nits :-) https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:96: * This view assumes that a peeking card will always be present when the cards UI is used. This documentation is no longer correct. Maybe just delete it. https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:110: // No spacing needed when |mCardsUiEnabled| is enabled. How about: "No scroll spacing needed when using the cards ui." https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:139: * @return nit: either delete or fully document this @return https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:141: public boolean allowCardToPeek() { nit: "isCardAllowedToPeek" seems more appropriate for this method https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:57: * @return Also same as for the other one: either document @return or delete it. https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:59: private boolean allowHeaderToTransition() { Same as for the other method, I think "isHeaderAllowedToTransition" would be more readable.
https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:141: public boolean allowCardToPeek() { On 2016/07/15 15:05:28, Michael van Ouwerkerk wrote: > nit: "isCardAllowedToPeek" seems more appropriate for this method (I think 'isCardPeekAllowed' is slightly nicer) https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:59: private boolean allowHeaderToTransition() { On 2016/07/15 15:05:28, Michael van Ouwerkerk wrote: > Same as for the other method, I think "isHeaderAllowedToTransition" would be > more readable. (again, maybe isHeaderTransitionAllowed instead)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:136: * Only allow the card to peek if the user has not scrolled on the page beyond the This seems more like an implementation comment, not a comment documenting the contract. https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:141: public boolean allowCardToPeek() { On 2016/07/15 15:08:23, PEConn1 wrote: > On 2016/07/15 15:05:28, Michael van Ouwerkerk wrote: > > nit: "isCardAllowedToPeek" seems more appropriate for this method > > (I think 'isCardPeekAllowed' is slightly nicer) To add my opinion about the prefered color of the bikeshed: Why even include "card" here, if the whole class is specific to cards? Why not just canPeek()?
PTAL https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:96: * This view assumes that a peeking card will always be present when the cards UI is used. On 2016/07/15 15:05:28, Michael van Ouwerkerk wrote: > This documentation is no longer correct. Maybe just delete it. Done. https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:110: // No spacing needed when |mCardsUiEnabled| is enabled. On 2016/07/15 15:05:28, Michael van Ouwerkerk wrote: > How about: "No scroll spacing needed when using the cards ui." Done. https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:136: * Only allow the card to peek if the user has not scrolled on the page beyond the On 2016/07/18 14:19:17, Bernhard Bauer wrote: > This seems more like an implementation comment, not a comment documenting the > contract. Done. https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:139: * @return On 2016/07/15 15:05:28, Michael van Ouwerkerk wrote: > nit: either delete or fully document this @return eclipse pulled a sneaky and added :) removed https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:141: public boolean allowCardToPeek() { On 2016/07/18 14:19:17, Bernhard Bauer wrote: > On 2016/07/15 15:08:23, PEConn1 wrote: > > On 2016/07/15 15:05:28, Michael van Ouwerkerk wrote: > > > nit: "isCardAllowedToPeek" seems more appropriate for this method > > > > (I think 'isCardPeekAllowed' is slightly nicer) > > To add my opinion about the prefered color of the bikeshed: Why even include > "card" here, if the whole class is specific to cards? Why not just canPeek()? After much deliberation I have gone with canPeek :) Thanks all https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:142: return mRecyclerView.computeVerticalScrollOffset() <= itemView.getHeight(); On 2016/07/15 14:03:26, PEConn1 wrote: > Can't we use functions from the RecyclerView's LayoutManager > (http://stackoverflow.com/a/25053500) ? Can you be a little more specific here please https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:57: * @return On 2016/07/15 15:05:28, Michael van Ouwerkerk wrote: > Also same as for the other one: either document @return or delete it. Done. https://codereview.chromium.org/2144413003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:59: private boolean allowHeaderToTransition() { On 2016/07/15 15:08:23, PEConn1 wrote: > On 2016/07/15 15:05:28, Michael van Ouwerkerk wrote: > > Same as for the other method, I think "isHeaderAllowedToTransition" would be > > more readable. > > (again, maybe isHeaderTransitionAllowed instead) Done.
https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:136: * Can the view holder card peek. Nit: "Returns whether the card can peek." https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:53: * Can the header transition from being shown to hidden. Nit: "Returns whether [...]". Also, could you explain what it means if the header can't transition? What state will it be in? An alternative might be to extract a method instead that will return the header height for all the three cases that we have.
PTAL https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:136: * Can the view holder card peek. On 2016/07/19 10:34:49, Bernhard Bauer wrote: > Nit: "Returns whether the card can peek." Maybe this is just for javascript but you never mention return in the comment as it is explicit. https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:53: * Can the header transition from being shown to hidden. On 2016/07/19 10:34:49, Bernhard Bauer wrote: > Nit: "Returns whether [...]". Also, could you explain what it means if the > header can't transition? What state will it be in? > > An alternative might be to extract a method instead that will return the header > height for all the three cases that we have. Done.
https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:136: * Can the view holder card peek. On 2016/07/19 11:21:59, mcwilliams wrote: > On 2016/07/19 10:34:49, Bernhard Bauer wrote: > > Nit: "Returns whether the card can peek." > > Maybe this is just for javascript but you never mention return in the comment as > it is explicit. The method above does, so we should update both. If we're going for succinctness, we could also add a @return tag: /** @return Whether the card can peek. */
PTAL https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:136: * Can the view holder card peek. On 2016/07/19 12:40:54, Bernhard Bauer wrote: > On 2016/07/19 11:21:59, mcwilliams wrote: > > On 2016/07/19 10:34:49, Bernhard Bauer wrote: > > > Nit: "Returns whether the card can peek." > > > > Maybe this is just for javascript but you never mention return in the comment > as > > it is explicit. > > The method above does, so we should update both. If we're going for > succinctness, we could also add a @return tag: > > /** @return Whether the card can peek. */ Done.
https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:131: * @return Is the card peeking. "@return Whether the card is peeking." You want this to complete a sentence that starts with "Returns [...]". https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:54: * @return Can the header transition. Same here -- "@return Whether the header can transition". https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:70: // If the height is not visible - set the height to 0. "If the _header_ is not visible"? Then again, this doesn't really provide any information that isn't already in the code. https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:71: headerHeight = 0; Just return directly here and get rid of the else statements.
PTAL https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:131: * @return Is the card peeking. On 2016/07/19 15:12:02, Bernhard Bauer wrote: > "@return Whether the card is peeking." You want this to complete a sentence that > starts with "Returns [...]". Done. https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:54: * @return Can the header transition. On 2016/07/19 15:12:02, Bernhard Bauer wrote: > Same here -- "@return Whether the header can transition". Done. https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:70: // If the height is not visible - set the height to 0. On 2016/07/19 15:12:02, Bernhard Bauer wrote: > "If the _header_ is not visible"? Then again, this doesn't really provide any > information that isn't already in the code. Done. https://codereview.chromium.org/2144413003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:71: headerHeight = 0; On 2016/07/19 15:12:02, Bernhard Bauer wrote: > Just return directly here and get rid of the else statements. Done.
Did you forget to upload a new patch set? :)
Yes, thanks
https://codereview.chromium.org/2144413003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:69: } else if (!canTransition()) { Remove the "else" if the previous branch returns. Then if you add empty lines before the comments, the individual parts are automatically separated quite nicely. You could then also inline the simpler if-statements like this one and the one before.
PTAL https://codereview.chromium.org/2144413003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java (right): https://codereview.chromium.org/2144413003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetHeaderViewHolder.java:69: } else if (!canTransition()) { On 2016/07/19 16:04:29, Bernhard Bauer wrote: > Remove the "else" if the previous branch returns. Then if you add empty lines > before the comments, the individual parts are automatically separated quite > nicely. > > You could then also inline the simpler if-statements like this one and the one > before. Done.
LGTM!
The CQ bit was checked by mcwilliams@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== NTP, peeking card and heading hiding constraints Only allow the card to peek and heading to hide when the above the fold section is fully rendered. video: https://drive.google.com/corp/drive/folders/0B1IgAIJ9cgizR29mUHN3WHJ4U2s BUG=626657 ========== to ========== NTP, peeking card and heading hiding constraints Only allow the card to peek and heading to hide when the above the fold section is fully rendered. video: https://drive.google.com/corp/drive/folders/0B1IgAIJ9cgizR29mUHN3WHJ4U2s BUG=626657 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== NTP, peeking card and heading hiding constraints Only allow the card to peek and heading to hide when the above the fold section is fully rendered. video: https://drive.google.com/corp/drive/folders/0B1IgAIJ9cgizR29mUHN3WHJ4U2s BUG=626657 ========== to ========== NTP, peeking card and heading hiding constraints Only allow the card to peek and heading to hide when the above the fold section is fully rendered. video: https://drive.google.com/corp/drive/folders/0B1IgAIJ9cgizR29mUHN3WHJ4U2s BUG=626657 Committed: https://crrev.com/cff297f895ebef1cbee11cecc5ab20183582d0d1 Cr-Commit-Position: refs/heads/master@{#406320} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/cff297f895ebef1cbee11cecc5ab20183582d0d1 Cr-Commit-Position: refs/heads/master@{#406320} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
