Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfeat(navbar): Add basic search bar component to site. #134
Conversation
|
Can you also add some tests? :D |
| } | ||
|
|
||
| public toggleIsExpanded() { | ||
| this._isExpanded = !this._isExpanded; |
kara
Mar 20, 2017
Contributor
The autocomplete trigger has a panelOpen property. Have you tried using that instead of toggling it here? You should be able to do a ViewChild query for 'auto' to get a reference to the trigger.
host: {
'[class.expanded]': '_autocompleteTrigger?.panelOpen'
}
The autocomplete trigger has a panelOpen property. Have you tried using that instead of toggling it here? You should be able to do a ViewChild query for 'auto' to get a reference to the trigger.
host: {
'[class.expanded]': '_autocompleteTrigger?.panelOpen'
}
riavalon
Mar 21, 2017
Author
Contributor
I believe this will cause the input to expand only when the user starts typing and suggesting show up. We want the input to enlarge right when the user focuses it so I'm using the _isExpanded here instead
I believe this will cause the input to expand only when the user starts typing and suggesting show up. We want the input to enlarge right when the user focuses it so I'm using the _isExpanded here instead
kara
Mar 21, 2017
Contributor
The autocomplete panel should appear whenever the input is focused. If that's not happening, seems like an autocomplete bug.
The autocomplete panel should appear whenever the input is focused. If that's not happening, seems like an autocomplete bug.
riavalon
Mar 22, 2017
Author
Contributor
I think I may be. It seems that the first time the field is focused it does not show the panel, and the second time the field is focused it does show it.
I think I may be. It seems that the first time the field is focused it does not show the panel, and the second time the field is focused it does show it.
| }) | ||
|
|
||
| export class SearchBar { | ||
| @HostBinding('class.expanded') _isExpanded = false; |
kara
Mar 20, 2017
Contributor
Move this to the host object in the Component metadata? Generally prefer that instead of using the decorator.
Move this to the host object in the Component metadata? Generally prefer that instead of using the decorator.
riavalon
Mar 20, 2017
Author
Contributor
I added it as a decorator as the docs on angular.io suggest it's better to prefer the decorator over the metadata: https://angular.io/docs/ts/latest/guide/style-guide.html#!#06-03
I added it as a decorator as the docs on angular.io suggest it's better to prefer the decorator over the metadata: https://angular.io/docs/ts/latest/guide/style-guide.html#!#06-03
kara
Mar 20, 2017
Contributor
In Material, we actually do the opposite because @HostBinding causes issues with Angular Universal (which it seems the docs authors weren't aware of)
In Material, we actually do the opposite because @HostBinding causes issues with Angular Universal (which it seems the docs authors weren't aware of)
riavalon
Mar 21, 2017
Author
Contributor
Ahh, I see. Alrighty, I'll switch it up 👍
Ahh, I see. Alrighty, I'll switch it up
| type="text" | ||
| (focus)="toggleIsExpanded(true)" | ||
| (blur)="toggleIsExpanded(false)" | ||
| (keyup.enter)="navigate($event.target.value.toLowerCase())" |
kara
Mar 20, 2017
•
Contributor
Does this also navigate when you click on the option? Also have you considered switching to the optionSelections observable on the trigger rather than this event? It emits any option object that is selected (via the keyboard or the mouse).
Does this also navigate when you click on the option? Also have you considered switching to the optionSelections observable on the trigger rather than this event? It emits any option object that is selected (via the keyboard or the mouse).
| </div> | ||
|
|
||
| <md-autocomplete #auto="mdAutocomplete"> | ||
| <md-option *ngFor="let item of filteredSuggestions | async" [value]="item.name"> |
kara
Mar 20, 2017
•
Contributor
Another idea would be to bind the entire item as the option value, and use the displayWith property on md-autocomplete to map that object to its display value (the name). That way, you don't have to go through the list in navigate() to find the ID that matches the search string. You can just get the object itself on selection and pass through its ID to the route.
Another idea would be to bind the entire item as the option value, and use the displayWith property on md-autocomplete to map that object to its display value (the name). That way, you don't have to go through the list in navigate() to find the ID that matches the search string. You can just get the object itself on selection and pass through its ID to the route.
| import {Router} from '@angular/router'; | ||
| import {FormControl} from '@angular/forms'; | ||
| import {MdSnackBar} from '@angular/material'; | ||
| import {Observable} from 'rxjs'; |
kara
Mar 20, 2017
Contributor
Can you restrict the import a bit? From 'rxjs/Observable'
Can you restrict the import a bit? From 'rxjs/Observable'
jelbourn
Mar 20, 2017
Member
To elaborate: when you import from just rxjs, it brings in a lot of than just what you're using and increases the payload size.
To elaborate: when you import from just rxjs, it brings in a lot of than just what you're using and increases the payload size.
| box-sizing: border-box; | ||
| } | ||
|
|
||
| &.expanded .search-input-container { |
jelbourn
Mar 20, 2017
Member
All of the css should start with a docs- prefix
(mainly so that people looking at the docs site in dev tools can differentiate between docs-site-specific css and the underlying library css)
All of the css should start with a docs- prefix
(mainly so that people looking at the docs site in dev tools can differentiate between docs-site-specific css and the underlying library css)
| &::-webkit-input-placeholder { @include color-placeholder(); } /* Chrome/Opera/Safari */ | ||
| &::-moz-placeholder { @include color-placeholder(); } /* Firefox 19+ */ | ||
| &:-moz-placeholder { @include color-placeholder(); } /* Firefox 18- */ | ||
| &:ms-input-placeholder { @include color-placeholder(); } /* IE 10+ */ |
jelbourn
Mar 20, 2017
Member
autoprefixer is supposed to be built into the angular-cli, but I'm not sure if there's some specific configuration needed to turn it on
autoprefixer is supposed to be built into the angular-cli, but I'm not sure if there's some specific configuration needed to turn it on
riavalon
Mar 20, 2017
Author
Contributor
I did see that it was auto-prefixing some styles, but the input placeholder one unfortunately wasn't being done for some reason.
I did see that it was auto-prefixing some styles, but the input placeholder one unfortunately wasn't being done for some reason.
| import {Router} from '@angular/router'; | ||
| import {FormControl} from '@angular/forms'; | ||
| import {MdSnackBar} from '@angular/material'; | ||
| import {Observable} from 'rxjs'; |
jelbourn
Mar 20, 2017
Member
To elaborate: when you import from just rxjs, it brings in a lot of than just what you're using and increases the payload size.
To elaborate: when you import from just rxjs, it brings in a lot of than just what you're using and increases the payload size.
| <svg fill="#FFFFFF" height="24" viewBox="0 0 24 24" width="24" xmlns="http://www.w3.org/2000/svg"> | ||
| <path d="M15.5 14h-.79l-.28-.27C15.41 12.59 16 11.11 16 9.5 16 5.91 13.09 3 9.5 3S3 5.91 3 9.5 5.91 16 9.5 16c1.61 0 3.09-.59 4.23-1.57l.27.28v.79l5 4.99L20.49 19l-4.99-5zm-6 0C7.01 14 5 11.99 5 9.5S7.01 5 9.5 5 14 7.01 14 9.5 11.99 14 9.5 14z"/> | ||
| <path d="M0 0h24v24H0z" fill="none"/> | ||
| </svg> |
jelbourn
Mar 20, 2017
Member
Is there a font icon for search that would serve for this instead of using an svg?
Is there a font icon for search that would serve for this instead of using an svg?
87a943c
to
a49b4ce
c1c7f81
to
f688a90
|
@jelbourn This should be ready for another look over again |
| placeholder="Search" | ||
| type="text" | ||
| (focus)="toggleIsExpanded(true)" | ||
| (blur)="toggleIsExpanded(false)" |
jelbourn
Jun 6, 2017
Member
This method doesn't take an argument
This method doesn't take an argument
| @@ -0,0 +1,60 @@ | |||
| $input-bg: #85D9E2; | |||
jelbourn
Jun 6, 2017
Member
All of the colors should be part of the theme
All of the colors should be part of the theme
| (keyup.enter)="handlePlainSearch($event.target.value.toLowerCase())" | ||
| [mdAutocomplete]="auto" | ||
| [formControl]="searchControl"> | ||
| <i class="material-icons">search</i> |
jelbourn
Jun 6, 2017
Member
This should probably be <md-icon>
This should probably be <md-icon>
| } | ||
| } | ||
|
|
||
| input { |
jelbourn
Jun 6, 2017
Member
Can this be done with a css class instead? E.g. .docs-search-input
Can this be done with a css class instead? E.g. .docs-search-input
| } | ||
|
|
||
| public handlePlainSearch(searchTerm) { | ||
| const item = this.allDocItems.find(item => item.name.toLowerCase() === searchTerm); |
jelbourn
Jun 6, 2017
•
Member
Partial matches should also jump to the first result. E.g., typing "Date" and hitting enter should jump to the datepicker.
Partial matches should also jump to the first result. E.g., typing "Date" and hitting enter should jump to the datepicker.
|
|
||
| public handlePlainSearch(searchTerm) { | ||
| const item = this.allDocItems.find(item => item.name.toLowerCase() === searchTerm); | ||
| item ? this.navigate(item.id) : this._showError(); |
jelbourn
Jun 6, 2017
Member
We should clear the input on a successful search
We should clear the input on a successful search
| @ViewChild(MdAutocompleteTrigger) | ||
| private _autocompleteTrigger: MdAutocompleteTrigger; | ||
|
|
||
| public allDocItems: DocItem[]; |
jelbourn
Jun 6, 2017
Member
Don't need to explicitly say public since that's the default
Don't need to explicitly say public since that's the default
| public allDocItems: DocItem[]; | ||
| public filteredSuggestions: Observable<DocItem[]>; | ||
| public searchControl: FormControl = new FormControl(''); | ||
| public sub; |
jelbourn
Jun 6, 2017
Member
sub should have a more specific name and probably a type
sub should have a more specific name and probably a type
|
@jelbourn Updated this with the recent requested changes |
|
@jelbourn Set the panel to open and close in sync with the expanding animation |
|
@riavalon looks like clicking on the autocomplete items doesn't navigate any more |
|
@jelbourn Blur event was being called when trying to click on an option. I rewrote the focus/blur logic to ignore clicks on |
|
Up, this would be a nice feature on the docs site |
note:
karma.conf.jsfile has errors in it preventing tests from running. The issue is fixed in this pr: #138