Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cdk/table) add directive to enable recycle view repeater #21508

Merged

Conversation

MichaelJamesParsons
Copy link
Collaborator

The CDK and Material tables use the dispose view repeater strategy, which creates new rows from scratch when the dataset changes. This PR adds directives that enable the recycle view repeater, which caches disposed rows and reuses them when the dataset changes. This strategy reduces rendering latency for most tables, but may not be compatible with tables that animate rows.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 6, 2021
@MichaelJamesParsons MichaelJamesParsons force-pushed the recycle-view-repeater-directive branch 2 times, most recently from c555c3b to 8e8f7fe Compare January 6, 2021 22:42
@MichaelJamesParsons MichaelJamesParsons added G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround area: cdk/table area: material/table labels Jan 6, 2021
@jelbourn jelbourn added the target: minor This PR is targeted for the next minor release label Jan 6, 2021
@MichaelJamesParsons MichaelJamesParsons force-pushed the recycle-view-repeater-directive branch 4 times, most recently from c0af9f7 to e81b022 Compare January 7, 2021 02:22
@jelbourn jelbourn requested a review from mmalerba January 7, 2021 22:26
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.

Overall looks good, just a couple small nits

Comment on lines 1 to 7
table {
width: 100%;
}

th {
text-align: left;
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add CSS classes to the example like .example-table and .example-row and use those here? I like to make it unambiguously clear where each styles comes from in examples so that anyone inspecting the example can quickly distinguish between example styles and component styles.

(similar for other examples)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Updated here and in the mat table demo.

* tables that animate rows.
*/
@Directive({
selector: 'cdk-table[cdkRecycleRows], table[cdk-table][cdkRecycleRows]',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
selector: 'cdk-table[cdkRecycleRows], table[cdk-table][cdkRecycleRows]',
selector: 'cdk-table[recycleRows], table[cdk-table][recycleRows]',

I would do just recycleRows since we're conceptually pretending that this is an option on CdkTable, the properties of which wouldn't be prefixed. Same for the mat version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Updated here and in the mat table.

Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

LGTM

Should there be changes to the documentation that explains these "inputs"?

@MichaelJamesParsons MichaelJamesParsons force-pushed the recycle-view-repeater-directive branch 2 times, most recently from d5a6ed1 to bab9edd Compare January 22, 2021 22:11
@MichaelJamesParsons MichaelJamesParsons force-pushed the recycle-view-repeater-directive branch from bab9edd to 46c9e7e Compare January 22, 2021 22:14
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

@MichaelJamesParsons would you be willing to send a follow-up PR with docs? (just adding a brief section to table.md)

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jan 26, 2021
@MichaelJamesParsons
Copy link
Collaborator Author

@jelbourn yes, I'll prepare a PR for the docs this week.

@annieyw annieyw merged commit f68662c into angular:master Feb 5, 2021
wagnermaciel pushed a commit to wagnermaciel/components that referenced this pull request Feb 8, 2021
wagnermaciel pushed a commit to wagnermaciel/components that referenced this pull request Feb 8, 2021
@MichaelJamesParsons MichaelJamesParsons deleted the recycle-view-repeater-directive branch February 16, 2021 19:18
@MichaelJamesParsons
Copy link
Collaborator Author

Documented in #21926

@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 Mar 19, 2021
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 area: cdk/table area: material/table cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants