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(autocomplete): support for md-optgroup #5604

Merged
merged 6 commits into from Aug 22, 2017

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jul 8, 2017

Adds support for md-optgroup in md-autocomplete by:

  • Changing the @ViewChild query to pick up descendant options.
  • Tweaking the keyboard scrolling to handle having group labels before options.

Fixes #5581.

@crisbeto crisbeto self-assigned this Jul 8, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 8, 2017
@crisbeto crisbeto assigned kara and unassigned crisbeto Jul 8, 2017

it('should scroll to active options below the fold', fakeAsync(() => {
tick();
const container = document.querySelector('.cdk-overlay-pane .mat-autocomplete-panel')!;
Copy link
Member

Choose a reason for hiding this comment

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

How about querying that element in the beforeEach? The tick() and container variable initialization is done in every test of the current describe.

Copy link
Member Author

@crisbeto crisbeto Jul 8, 2017

Choose a reason for hiding this comment

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

Good point. Done.

<input mdInput placeholder="State" [mdAutocomplete]="auto" [(ngModel)]="selectedState">
</md-input-container>

<md-autocomplete #auto="mdAutocomplete">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an autocomplete like this to the autocomplete demo page?

const groups = this.autocomplete.optionGroups.toArray();
let groupCounter = 0;

for (let i = 0; i < optionIndex + 1; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This process looks pretty similar to how select calculates option index here. Given this is a common operation for option groups, maybe we could move this into a reusable function in core/option? A bit DRY-er.

Copy link
Member Author

@crisbeto crisbeto Jul 24, 2017

Choose a reason for hiding this comment

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

I'm not sure what that would even be called, countGroupLabelsBeforeOption? I would rather put it as a static method on the MdOption instead since having it float around as a function implies that it's a bit more reusable.

Copy link
Contributor

Choose a reason for hiding this comment

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

countGroupsBeforeOption sounds reasonable, but open to having it as a static method on MdOption too.

@kara
Copy link
Contributor

kara commented Jul 24, 2017

Can we also add a blurb to the autocomplete docs, so people know about this feature?

@kara kara assigned crisbeto and unassigned kara Jul 24, 2017
@crisbeto crisbeto assigned kara and unassigned crisbeto Jul 25, 2017
@crisbeto
Copy link
Member Author

Added all the changes as discussed.

Copy link
Contributor

@kara kara 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 needs a rebase.

@kara kara assigned crisbeto and unassigned kara Aug 9, 2017
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Aug 9, 2017
Adds support for `md-optgroup` in `md-autocomplete` by:
* Changing the `@ViewChild` query to pick up descendant options.
* Tweaking the keyboard scrolling to handle having group labels before options.

Fixes angular#5581.
name: string;
}

interface StateGroup {
Copy link
Contributor

Choose a reason for hiding this comment

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

Failing presubmit because of TS error for "private name StateGroup" and "private name State". I think you need to export these interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also can you fix the lint failure? https://travis-ci.org/angular/material2/jobs/266064783

@kara kara added pr: needs cleanup / tests presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged and removed action: merge The PR is ready for merge by the caretaker labels Aug 21, 2017
@crisbeto
Copy link
Member Author

Fixed @kara.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs cleanup / tests presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels Aug 21, 2017
@kara kara merged commit e41d0f3 into angular:master Aug 22, 2017
@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 6, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Md-autocomplete always hidden when used with md-optgroup
4 participants