[Reading List] Fix lazy loading for reading list [chromium/src : main]
0 views
Skip to first unread message
Alison Gale (Gerrit)
4:02 PM (5 hours ago) 4:02 PM
to Rebekah Potter, Chromium LUCI CQ, chromium...@chromium.org
Attention needed from Rebekah Potter
Alison Gale voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Rebekah Potter
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
6:28 PM (3 hours ago) 6:28 PM
to Alison Gale, Chromium LUCI CQ, chromium...@chromium.org
Attention needed from Alison Gale
Rebekah Potter voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Code-Review | +1 |
2 comments
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Rebekah Potter . resolved
LGTM with a nit. This looks a lot simpler.
File chrome/browser/resources/side_panel/reading_list/reading_list_app.html.ts
Line 23, Patchset 2 (Latest):
<div id="scroller" class="sp-scroller sp-scroller-top-of-page" ?hidden="${
!this.getAllItems_().length}">Rebekah Potter . unresolved
can we put this full attribute on a new line, indented +4 per the style guide, instead of breaking it up?
May want to disable clang-format for the template using clang-format off/clang-format on comments (I have a CL out for review to turn off clang format for .html.ts files, but needs one more approval before it can land).
Related details
Attention is currently required from:
- Alison Gale
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Gale (Gerrit)
6:44 PM (3 hours ago) 6:44 PM
to Rebekah Potter, Chromium LUCI CQ, chromium...@chromium.org
Alison Gale added 1 comment![]( "Open in Gerrit")
File chrome/browser/resources/side_panel/reading_list/reading_list_app.html.ts
Line 23, Patchset 2:
<div id="scroller" class="sp-scroller sp-scroller-top-of-page" ?hidden="${
!this.getAllItems_().length}">Rebekah Potter . resolved
can we put this full attribute on a new line, indented +4 per the style guide, instead of breaking it up?
May want to disable clang-format for the template using clang-format off/clang-format on comments (I have a CL out for review to turn off clang format for .html.ts files, but needs one more approval before it can land).
Alison Gale
Oh yeah, that's a good call. Updated
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Gale (Gerrit)
6:44 PM (3 hours ago) 6:44 PM
to Rebekah Potter, Chromium LUCI CQ, chromium...@chromium.org
Alison Gale voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
8:45 PM (1 hour ago) 8:45 PM
to Alison Gale, Rebekah Potter, chromium...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/resources/side_panel/reading_list/reading_list_app.html.ts
Insertions: 4, Deletions: 2.
@@ -8,6 +8,7 @@
import type {ReadingListAppElement} from './reading_list_app.js';
export function getHtml(this: ReadingListAppElement) {
+ // clang-format off
return html`<!--_html_template_start_-->
<div id="content" ?hidden="${this.loadingContent_}">
<div class="sp-scroller sp-scroller-top-of-page"
@@ -20,8 +21,8 @@
</sp-empty-state>
</div>
- <div id="scroller" class="sp-scroller sp-scroller-top-of-page" ?hidden="${
- !this.getAllItems_().length}">
+ <div id="scroller" class="sp-scroller sp-scroller-top-of-page"
+ ?hidden="${!this.getAllItems_().length}">
<cr-lazy-list id="readingListList" class="sp-card"
.items="${this.getAllItems_()}"
.itemSize="${this.itemSize_}"
@@ -69,4 +70,5 @@
</sp-footer>
</div>
<!--_html_template_end_-->`;
+ // clang-format on
}
```
Change information
Commit message:
[Reading List] Fix lazy loading for reading list
Have the height calculated from the sp-scroller element rather than
trying to pass the viewport height. Confirmed locally that only the
items in the viewport are rendered rather than the full list.
Bug: 532553456
Change-Id: I861c933e8bfdeca03174da0ad9d7b077b914bbbb
Reviewed-by: Rebekah Potter <rbpo...@chromium.org>
Commit-Queue: Alison Gale <ag...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1659175}
Files:
- M chrome/browser/resources/side_panel/reading_list/reading_list_app.css
- M chrome/browser/resources/side_panel/reading_list/reading_list_app.html.ts
- M chrome/browser/resources/side_panel/reading_list/reading_list_app.ts
Change size: S
Delta: 3 files changed, 14 insertions(+), 18 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Rebekah Potter
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages