Skip to content

perf(list-base): convert list-base adapter to class object #19983

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

perf(list-base): convert list-base adapter to class object #19983

wants to merge 4 commits into from

Conversation

ngwattcos
Copy link
Contributor

@ngwattcos ngwattcos commented Jul 15, 2020 •
edited
Loading

Converts list-base adapter to class object to hopefully memory usage and increase performance

Test failures hopefully should be resolved after this PR is merged:
material-components/material-components-web#6256

@ngwattcos
perf(list-base): convert list-base adapter to class object
9fbd828 
Converts list-base adapter to class object to hopefully reduce memory usage
@ngwattcos ngwattcos requested a review from mmalerba as a code owner July 15, 2020 03:12
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 15, 2020
@mmalerba
Copy link
Contributor

@andrewseguin @jelbourn should we adopt this style for all of the MDC adapters? We still need an instance per instance of the component, but at least all of the methods live on a single instance of the prototype this way

@crisbeto
Copy link
Member

I think the object literal pattern is used by MDC internally too. Should we sync up with them?

mmalerba
mmalerba approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

Yeah, we should probably bring it up with the MDC team, they might want to make the same change

LGTM

@mmalerba mmalerba added lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jul 15, 2020
@ngwattcos ngwattcos requested a review from crisbeto as a code owner July 15, 2020 22:24
jelbourn
jelbourn reviewed Jul 15, 2020
View reviewed changes
src/material-experimental/mdc-checkbox/checkbox.ts Outdated
@@ -57,6 +57,46 @@ const RIPPLE_ANIMATION_CONFIG: RippleAnimationConfig = {
exitDuration: numbers.FG_DEACTIVATION_MS,
};

/** Singleton check box adapter. */
class CheckBoxAdapter implements MDCCheckboxAdapter {
Copy link
Member

@jelbourn jelbourn Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would put the adapter classes in a separate file (each)

@ngwattcos ngwattcos force-pushed the optimize-list-base branch from 1f77a5b to 73fc0b1 Compare July 16, 2020 00:11
@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Jul 16, 2020
mmalerba
mmalerba reviewed Jul 16, 2020
View reviewed changes
@ngwattcos
refactor(list-base): Fix linting issues
3ff078d 
Fix linting issues and convert adapter method style to class methods
@ngwattcos ngwattcos force-pushed the optimize-list-base branch from 73fc0b1 to 3ff078d Compare July 16, 2020 21:46
mmalerba
mmalerba reviewed Jul 20, 2020
View reviewed changes
mmalerba
mmalerba reviewed Jul 21, 2020
View reviewed changes
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@andrewseguin
Copy link
Contributor

@ngwattcos Would you like to continue with this PR? If so, can you rebase it?

@andrewseguin andrewseguin added the needs: clarification The issue does not contain enough information for the team to determine if it is a real bug label Mar 24, 2022
@andrewseguin
Copy link
Contributor

Our strategy going forward with adapters is changing soon that will make this PR obsolete

@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 May 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@mmalerba mmalerba mmalerba approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@crisbeto crisbeto Awaiting requested review from crisbeto

Assignees
No one assigned
Labels
needs: clarification The issue does not contain enough information for the team to determine if it is a real bug target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ngwattcos @mmalerba @crisbeto @andrewseguin @jelbourn @googlebot