Skip to content
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

feat(material-experimental/mdc-autocomplete): implement MDC-based mat-autocomplete #20247

Merged
merged 1 commit into from Aug 13, 2020

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Aug 8, 2020

  • Moves all of the autocomplete logic into base classes so that it can be reused between the standard and MDC components.
  • Re-implements mat-autocomplete using the logic from the existing one and the styling from MDC.

The MDC-based autocomplete behaves identically to the existing one, with the only minor difference being that MDC one fixes a long-standing issue where we expect a hardcoded height for each of the options. It was easier to fix the bug and add logic to support arbitrary option heights than to add more logic to account for MDC's styles.

Note: I will make a follow-up PR that adds test harnesses for the new module. I didn't add them here, because the PR is fairly large as it is.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 8, 2020
export declare const MAT_AUTOCOMPLETE_VALUE_ACCESSOR: any;

export declare class MatAutocomplete extends _MatAutocompleteMixinBase implements AfterContentInit, CanDisableRipple, OnDestroy {
export declare abstract class _MatAutocompleteBase extends _MatAutocompleteMixinBase implements AfterContentInit, CanDisableRipple, OnDestroy {
Copy link
Member Author

Choose a reason for hiding this comment

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

The API differences here come from the fact that I moved all the logic into a base class, but everything should be backwards-compatible.

@crisbeto crisbeto force-pushed the mdc-autocomplete branch 2 times, most recently from 50adb1c to 10382e6 Compare August 8, 2020 15:06
@crisbeto crisbeto marked this pull request as ready for review August 8, 2020 15:19
@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release labels Aug 8, 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 comments on comments

…-autocomplete

* Moves all of the autocomplete logic into base classes so that it can be reused between the standard and MDC components.
* Re-implements `mat-autocomplete` using the logic from the existing one and the styling from MDC.

The MDC-based autocomplete behaves identically to the existing one, with the only minor difference being that MDC one fixes a long-standing issue where we expect a hardcoded height for each of the options. It was easier to fix the bug and add logic to support arbitrary option heights than to add more logic to account for MDC's styles.
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Aug 11, 2020
@andrewseguin andrewseguin merged commit c8f03c7 into angular:master Aug 13, 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 13, 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 P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants