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

docs(autocomplete): Adds option group example #10666

Merged
merged 2 commits into from Apr 18, 2018

Conversation

Projects
None yet
6 participants
@raygervais
Contributor

raygervais commented Apr 2, 2018

fixes #10196,
StackBlitz example: https://stackblitz.com/edit/angular-smg2xm?file=app/app.component.ts

Adds a option group example which displays ideal logic and best practices for the material autocomplete component when using said groups.

docs(autocomplete): Adds option group example
Adds a option group example which displays ideal logic and best
practices for the material autocomplete component.

@raygervais raygervais requested review from amcdnl, crisbeto, jelbourn and kara as code owners Apr 2, 2018

@googlebot googlebot added the cla: yes label Apr 2, 2018

</mat-optgroup>
</mat-autocomplete>
</mat-form-field>
</div>

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

Unnecessary closing tag.

@@ -0,0 +1,13 @@
<form [formGroup]="animalForm" novalidate>
<mat-form-field>

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

Indentation seems to be wrong here.

<input type="text" matInput placeholder="Species Group" formControlName="speciesGroup" required [matAutocomplete]="autoGroup"/>
<mat-autocomplete #autoGroup="matAutocomplete">
<mat-optgroup *ngFor="let group of speciesGroupeOptions | async" [label]="group.letter">
<mat-option *ngFor="let name of group.name" [value]="name">

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

Extra space before the value binding.

</mat-autocomplete>
</mat-form-field>
</div>
</form>

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

Needs an extra line at the end of the file.

@@ -0,0 +1,63 @@
import { Component, OnInit } from '@angular/core';
import { FormGroup, FormBuilder, Validators } from '@angular/forms';

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

Spacing around the imports is inconsistent.

class SpeciesGroup {
letter:string;
name: string[];
}

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

Missing a newline at the end of the file.

}
class SpeciesGroup {
letter:string;

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

Needs a space after letter:.

class SpeciesGroup {
letter:string;
name: string[];

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

Since this is an array it should be called names.

buildForm() {
this.animalForm = this.fb.group({
species: ['', [Validators.required]],

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

I don't think that we should start putting validation on this. We try to keep the examples as simple as possible and in this case the validation doesn't add anything.

filterGroup(val: string): SpeciesGroup[] {
if (val) {
return this.speciesGroup
.map(group => ({ letter: group.letter, name: this._filter(group.name, val) }))

This comment has been minimized.

@crisbeto

crisbeto Apr 2, 2018

Member

The map and the filter need some indentation.

@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 2, 2018

Fixed @crisbeto, I also forked the original StackBlitz and made the updates here for visual testing / inspection. Thanks for pointing out the inconsistencies, it appears I need more coffee in my system before I touch a keyboard again ☕️

import { Component, OnInit } from '@angular/core';
import { FormGroup, FormBuilder, Validators } from '@angular/forms';
import { Observable } from 'rxjs/Observable';
import { startWith } from 'rxjs/operators/startWith';

This comment has been minimized.

@crisbeto

crisbeto Apr 3, 2018

Member

In general we don't do spaces around the imports. E.g. these should be import {startWith} from 'rxjs/operators/startWith';

ngOnInit() {
this.stateGroupOptions = this.stateForm.get('stateGroup').valueChanges
.pipe(

This comment has been minimized.

@crisbeto

crisbeto Apr 3, 2018

Member

The .pipe should either be on the previous line or be indented.

stateGroupOptions: Observable<StateGroup[]>;
constructor(private fb: FormBuilder) {
this.stateForm = this.fb.group({

This comment has been minimized.

@crisbeto

crisbeto Apr 3, 2018

Member

You can initialize this one when you declare the property.

names: ['Vermont', 'Virginia']
}, {
letter: 'W',
names: ['Washingto n', 'West Virginia', 'Wisconsin', 'Wyoming']

This comment has been minimized.

@crisbeto

crisbeto Apr 3, 2018

Member

Extra space before the "n".

@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 4, 2018

Thanks for the reviews @crisbeto, I do apologize for the little items such as the spacing which really I should be catching as well. Will make note to improve and be diligent about that.

@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 4, 2018

Just noticed the lint and e2e failures, will address today.

@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 9, 2018

Sorry for the delay, I've been wrestling school obligations and apartment hunting prior to starting a new job. Does anyone have any idea how to fix the lint error here? I have not seen it before:

ERROR: /home/travis/build/angular/material2/src/material-examples/autocomplete-optgroup/autocomplete-optgroup-example.ts[2, 33]: 'Validators' is declared but its value is never read.

Here is the code that I'm trying to use:

stateForm: FormGroup = this.fb.group({
  stateGroup: '',
});
@rafaelss95

This comment has been minimized.

Contributor

rafaelss95 commented Apr 9, 2018

Just remove Validators from import.

@@ -0,0 +1,12 @@
<form [formGroup]="stateForm" novalidate>

This comment has been minimized.

@rafaelss95

rafaelss95 Apr 9, 2018

Contributor

novalidate isn't necessary.

}
}
class StateGroup {

This comment has been minimized.

@rafaelss95

rafaelss95 Apr 9, 2018

Contributor

You could replace this by an Interface and move this to the top of the file (after imports);

docs(autocomplete): Fixes option group example
Changes code style and logic to expected logic and styleguide
@raygervais

This comment has been minimized.

Contributor

raygervais commented Apr 16, 2018

Thanks for the tips @rafaelss95, and sorry for the delay. I've fixed the Lint / E2E issues, and the failing test case relates to Material Table. If there is any other fix needed, I'd be happy to correct 👍

@crisbeto

LGTM

@mmalerba mmalerba merged commit 1cc0c7f into angular:master Apr 18, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci: build Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment