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-select): implement MDC-based mat-select #20502

Merged
merged 1 commit into from Sep 18, 2020

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Sep 6, 2020

Implements mat-select on top of MDC Web's styles. The new select is functionally identical to the current one since both are implemented on top of the same base class, but they have the following differences:

  • The logic that tries to position the selected option on top of the trigger has been removed from the MDC version. Now we only position the panel above or below, depending on the available space.
  • Since we don't have to deal with the panel positioning logic anymore, we can now allow the text of mat-option to wrap to multiple lines inside the MDC version.
  • The animations of the MDC version are simplified to mimic the ones from mat-menu, because the previous animations were tightly coupled to the old overlay positioning logic.

Aside from the new select, this PR includes the following fixes:

  • The MatOptionSelectionChange.source is now typed to the generic _MatOptionBase rather than the non-MDC implementation. This was an oversight when the base class was first implemented.
  • Uses min-height instead of height for the MDC mat-option in order to handle multi-line text better.
  • Adds the mat-form-field-hide-placeholder, mat-form-field-appearance-outline and mat-form-field-appearance-fill classes which were missing from the MDC form field. They're necessary so that the select can be styled to look correctly across all supported use cases.
  • The dev app for the select has been reorganized to emphasize mat-select, instead of mat-form-field with a native select. I've also cleaned up the styles so everything aligns better and is nicer to look at.

Note: I'll add the test harness as a follow-up since the PR is fairly large already.

Angular_Material_-_Google_Chrome_2020-09-06_14-02-12

@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 Sep 6, 2020
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 6, 2020
export declare const MAT_SELECT_TRIGGER: InjectionToken<MatSelectTrigger>;

export declare class MatSelect extends _MatSelectMixinBase implements AfterContentInit, OnChanges, OnDestroy, OnInit, DoCheck, ControlValueAccessor, CanDisable, HasTabIndex, MatFormFieldControl<any>, CanUpdateErrorState, CanDisableRipple {
export declare abstract class _MatSelectBase extends _MatSelectMixinBase implements AfterContentInit, OnChanges, OnDestroy, OnInit, DoCheck, ControlValueAccessor, CanDisable, HasTabIndex, MatFormFieldControl<any>, CanUpdateErrorState, CanDisableRipple {
Copy link
Member Author

@crisbeto crisbeto Sep 6, 2020

Choose a reason for hiding this comment

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

All of these API changes are due to me moving the logic into a base class.

@crisbeto crisbeto force-pushed the mdc-select branch 3 times, most recently from 150d402 to 2aa1adc Compare September 6, 2020 12:28
@crisbeto
Copy link
Member Author

The feedback has been addressed.

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.

LGTM

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Sep 10, 2020
@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Sep 10, 2020
@mmalerba
Copy link
Contributor

Please address the last comment before marking merge ready

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Sep 10, 2020
@mmalerba mmalerba added the G This is is related to a Google internal issue label Sep 14, 2020
@wagnermaciel
Copy link
Contributor

  • The MatOptionSelectionChange.source is now typed to the generic _MatOptionBase rather than the non-MDC implementation. This was an oversight when the base class was first implemented.

@crisbeto Is _MatSelectBase meant to be a public type? I ask because there is at least one instance internally where we had
const matSelect: MatSelect = event.source;
which would now be
const matSelect: _MatSelectBase = event.source;

@crisbeto
Copy link
Member Author

@wagnermaciel does that case throw a compilation error? I would've expected it not to complain since MatSelect extends _MatSelectBase. The base class technically isn't a public API, but I used it in the event, because we reuse the same event object for both select variants. Let me know if it's blocking you and I can find a different way around the issue (probably just duplicating the event).

@wagnermaciel
Copy link
Contributor

@crisbeto Yes, I ended up getting a compilation error shortly after writing that comment but I didn't get a chance to update you. It is blocking so please let me know what that line should now look like

@crisbeto
Copy link
Member Author

I've reworked it to avoid the compilation error.

Implements `mat-select` on top of MDC Web's styles. The new select is functionally
identical to the current one since both are implemented on top of the same base
class, but they have the following differences:
* The logic that tries to position the selected option on top of the trigger has
been removed from the MDC version. Now we only position the panel above or below,
depending on the available space.
* Since we don't have to deal with the panel positioning logic anymore, we can now
 allow the text of `mat-option` to wrap to multiple lines inside the MDC version.
* The animations of the MDC version are simplified to mimic the ones from `mat-menu`,
because the previous animations where tightly coupled to the old overlay positioning logic.

Aside from the new select, this PR includes the following fixes:
* The `MatOptionSelectionChange.source` is now typed to the generic `_MatOptionBase` rather
than the non-MDC implementation. This was an oversight when the base class was first implemented.
* Uses `min-height` instead of `height` for the MDC `mat-option` in order to handle multi-line
text better.
* Adds the `mat-form-field-hide-placeholder`, `mat-form-field-appearance-outline` and
`mat-form-field-appearance-fill` classes which were missing from the MDC form field. They're
necessary so that the select can be styled to look correctly across all supported use cases.
* The dev app for the select has been reorganized to emphasize `mat-select`, instead of
`mat-form-field` with a native `select`. I've also cleaned up the styles so everything aligns
better and nicer to look at.
@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 Oct 19, 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 G This is is related to a Google internal issue 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

6 participants