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
perf(table): Slightly improve speed of adding/remvoing sticky styles #19823
Conversation
@@ -32,4 +32,7 @@ import {ChangeDetectionStrategy, Component, ViewEncapsulation} from '@angular/co | |||
export class MatTable<T> extends CdkTable<T> { | |||
/** Overrides the sticky CSS class set by the `CdkTable`. */ | |||
protected stickyCssClass = 'mat-table-sticky'; | |||
|
|||
/** Overrides the need to add position: sticky on every sticky cell element in `CdkTable`. */ | |||
protected needsPositionStickyOnElement = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewseguin is there any reason we couldn't just always use a css class for this instead of attaching styles directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm up for making that change now, though I fear it could be breaking for any projects that have subclassed CdkTable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it feels to me like an internal implementation detail, but I'm curious to get @andrewseguin's take
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a reason - we should have a follow up PR that attempts to switch to just the class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I definitely want to try just making this the only behavior in a follow-up; if it passes TGP when I wouldn't consider it a breaking change
@kseamon can you rebase this PR? |
8cdb0c9
to
10e6a56
Compare
10e6a56
to
7977e23
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
In the longer run, it'd be nice to remove the addition of inline styles entirely (and have some simple CSS attached to CdkTable).