Skip to content

perf(int-bar): Convert MatInkBarFoundation adapter to class object - #19986

Merged
andrewseguin merged 3 commits into
angular:masterfrom
ngwattcos:optimize-ink-bar
Jul 20, 2020
Merged

perf(int-bar): Convert MatInkBarFoundation adapter to class object#19986
andrewseguin merged 3 commits into
angular:masterfrom
ngwattcos:optimize-ink-bar

Conversation

@ngwattcos

Copy link
Copy Markdown
Contributor

Converts the adapter to class object to hopefully reduce memory usage. Adds getters to access private fields.

Converts the adapter to class object to hopefully reduce memory usage. Adds getters to access private fields.
@ngwattcos
ngwattcos requested a review from crisbeto as a code owner July 15, 2020 03:17
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 15, 2020
getDestroyed() {
return this._destroyed;
}

Copy link
Copy Markdown
Member

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.

@ngwattcos ngwattcos Jul 17, 2020

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Exactly.

@crisbeto crisbeto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@crisbeto crisbeto added lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Jul 18, 2020
@andrewseguin
andrewseguin merged commit 78dc50b into angular:master Jul 20, 2020
andrewseguin pushed a commit that referenced this pull request Jul 20, 2020
…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)
ngwattcos added a commit to ngwattcos/components that referenced this pull request Jul 20, 2020
…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
wagnermaciel added a commit to wagnermaciel/components that referenced this pull request Jul 21, 2020
…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.
wagnermaciel added a commit that referenced this pull request Jul 21, 2020
…bject (#19986)" (#20051)

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.
wagnermaciel added a commit that referenced this pull request Jul 21, 2020
…bject (#19986)" (#20051)

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.

(cherry picked from commit 4a1b24c)
@mmalerba

Copy link
Copy Markdown
Contributor

@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

andrewseguin pushed a commit that referenced this pull request Jul 21, 2020
…bject (#19986)" (#20051)

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.

(cherry picked from commit 4a1b24c)
} from '@material/tab-indicator';

class TabIndicatorAdapter implements MDCTabIndicatorAdapter {
constructor(private readonly _delegate: MatInkBarFoundation) {}

Copy link
Copy Markdown
Contributor

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

Comment thread src/material-experimental/mdc-tabs/ink-bar.ts
@angular-automatic-lock-bot

Copy link
Copy Markdown

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 Aug 21, 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 P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants