Skip to content

fix,perf(column-resize): Coalesce style updates along with sticky styler #20086

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

Merged

Conversation

kseamon
Copy link
Collaborator

@kseamon kseamon commented Jul 24, 2020

Coalesces style updates triggered by the column resize system to trigger less layout thrashing. Also updates sticky column styles when resizing a column.

The net effect is a substantial improvement when applying min/max sizes at startup and a bit of a wash when actively resizing. The gains in active resize performance are mostly offset by the additional cost of recomputing sticky styles (but it was a bug before that they were not being recomputed). Should be much better for tables without sticky columns though.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 24, 2020
@kseamon kseamon changed the title Table coalesce measurements also resize fix,perf(column-resize) Jul 24, 2020
@kseamon kseamon changed the title fix,perf(column-resize) fix,perf(column-resize): Coalesce style updates along with sticky styler Jul 24, 2020
@kseamon kseamon force-pushed the table-coalesce-measurements-also-resize branch from bd05991 to 23569ea Compare July 28, 2020 17:11
@kseamon kseamon marked this pull request as ready for review July 28, 2020 17:27
@kseamon kseamon requested a review from andrewseguin as a code owner July 28, 2020 17:27
@kseamon kseamon requested a review from jelbourn July 28, 2020 21:44
@jelbourn jelbourn 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 perf This issue is related to performance labels Jul 28, 2020
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, just a couple nits

Also want @andrewseguin to take a look

@jelbourn jelbourn added lgtm merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed target: patch This PR is targeted for the next patch release labels Jul 28, 2020
@andrewseguin andrewseguin added action: merge The PR is ready for merge by the caretaker needs rebase labels Jul 31, 2020
@kseamon kseamon force-pushed the table-coalesce-measurements-also-resize branch from b74e9b3 to 39c1d99 Compare July 31, 2020 18:33
@kseamon kseamon force-pushed the table-coalesce-measurements-also-resize branch from 39c1d99 to db08c35 Compare July 31, 2020 18:37
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@mmalerba mmalerba merged commit e2bc083 into angular:master Aug 5, 2020
mmalerba pushed a commit that referenced this pull request Aug 5, 2020
@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 5, 2020
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 G This is is related to a Google internal issue merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed P2 The issue is important to a large percentage of users, with a workaround perf This issue is related to performance target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants