-
Notifications
You must be signed in to change notification settings - Fork 6.8k
perf(int-bar): Convert MatInkBarFoundation adapter to class object #19986
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
Conversation
Converts the adapter to class object to hopefully reduce memory usage. Adds getters to access private fields.
| getDestroyed() { | ||
| return this._destroyed; | ||
| } | ||
|
|
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.
Instead of adding getters for all of these, we can use readonly properties instead which won't generate any extra runtime code. If we need to change one of them (e.g. destroyed), we can do something like this (this as {destroyed: boolean}).destroyed = true.
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.
@crisbeto So to be clear, the properties _inkBarContentElement, _hostElement, and _getDestroyed should be exposed publicly but made readonly?
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.
Exactly.
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
…19986) * perf(int-bar): Convert MatInkBarFoundation adapter to class object Converts the adapter to class object to hopefully reduce memory usage. Adds getters to access private fields. * refactor(ink-bar): expose some fields and make public; remove getter methods * fix(ink-bar): fix build/typing issues in reassigning fields due to readonly (cherry picked from commit 78dc50b)
…ngular#19986) * perf(int-bar): Convert MatInkBarFoundation adapter to class object Converts the adapter to class object to hopefully reduce memory usage. Adds getters to access private fields. * refactor(ink-bar): expose some fields and make public; remove getter methods * fix(ink-bar): fix build/typing issues in reassigning fields due to readonly
…bject (angular#19986)" This reverts commit 78dc50b. This PR was marked merge-safe but caused presubmits to break. Tests were failing because the dev-app doesn't show ink bars anymore.
|
@ngwattcos It looks like this got reverted, please make the same stylistic changes I've been mentioning in your other similar PRs before it goes in again |
| @@ -13,6 +13,30 @@ import { | |||
| MDCTabIndicatorFoundation | |||
| } from '@material/tab-indicator'; | |||
|
|
|||
| class TabIndicatorAdapter implements MDCTabIndicatorAdapter { | |||
| constructor(private readonly _delegate: MatInkBarFoundation) {} | |||
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.
As with other PRs, put newlines between methods
|
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. |
Converts the adapter to class object to hopefully reduce memory usage. Adds getters to access private fields.