Skip to content

Conversation

@mmalerba
Copy link
Contributor

No description provided.

@mmalerba mmalerba added P2 The issue is important to a large percentage of users, with a workaround docs This issue is related to documentation labels Sep 27, 2018
@mmalerba mmalerba requested a review from jelbourn September 27, 2018 00:05
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 27, 2018
@mmalerba mmalerba added pr: merge safe target: patch This PR is targeted for the next patch release labels Sep 27, 2018
}

.example-alternate {
background: rgba(0, 0, 0, .2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may end up with white text on a white background if somebody selected a dark theme in the docs site.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think our examples really support theme styles, but I'll use a 50% gray so it at least looks ok in either.

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

changeDetection: ChangeDetectionStrategy.OnPush,
})
export class CdkVirtualScrollContextExample {
items = Array.from({length: 100000}).map((_, i) => `Item #${i}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can be reduced to new Array(100000).fill(0).map((_, i) => Item #${i}) which is slightly shorter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I originally had that but switched it to this. This feels more readable to me, so I think its better for the purposes of an example.

@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Sep 27, 2018
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn merged commit 5321ab3 into angular:master Sep 27, 2018
roboshoes pushed a commit to roboshoes/material2 that referenced this pull request Oct 23, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement docs This issue is related to documentation P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants