feat(navbar): Add basic search bar component to site. #134

riavalon · 2017-03-17T18:26:38Z

note: karma.conf.js file has errors in it preventing tests from running. The issue is fixed in this pr: #138

kara

Can you also add some tests? :D

kara · 2017-03-20T18:43:13Z

src/app/shared/navbar/searchbar/searchbar.ts

+  }
+
+  public toggleIsExpanded() {
+    this._isExpanded = !this._isExpanded;


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' }

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

The autocomplete panel should appear whenever the input is focused. If that's not happening, seems like an autocomplete bug.

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.

kara · 2017-03-20T18:44:37Z

src/app/shared/navbar/searchbar/searchbar.ts

+})
+
+export class SearchBar {
+  @HostBinding('class.expanded') _isExpanded = false;


Move this to the host object in the Component metadata? Generally prefer that instead of using the decorator.

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

In Material, we actually do the opposite because @HostBinding causes issues with Angular Universal (which it seems the docs authors weren't aware of)

Ahh, I see. Alrighty, I'll switch it up 👍

kara · 2017-03-20T18:48:26Z

src/app/shared/navbar/searchbar/searchbar.html

+    type="text"
+    (focus)="toggleIsExpanded(true)"
+    (blur)="toggleIsExpanded(false)"
+    (keyup.enter)="navigate($event.target.value.toLowerCase())"


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).

kara · 2017-03-20T18:52:19Z

src/app/shared/navbar/searchbar/searchbar.html

+</div>
+
+<md-autocomplete #auto="mdAutocomplete">
+  <md-option *ngFor="let item of filteredSuggestions | async" [value]="item.name">


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.

kara · 2017-03-20T18:52:57Z

src/app/shared/navbar/searchbar/searchbar.ts

+import {Router} from '@angular/router';
+import {FormControl} from '@angular/forms';
+import {MdSnackBar} from '@angular/material';
+import {Observable} from 'rxjs';


Can you restrict the import a bit? From 'rxjs/Observable'

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.

jelbourn · 2017-03-20T19:22:33Z

src/app/shared/navbar/searchbar/searchbar.scss

+    box-sizing: border-box;
+  }
+
+  &.expanded .search-input-container {


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)

jelbourn · 2017-03-20T19:26:36Z

src/app/shared/navbar/searchbar/searchbar.scss

+    &::-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+ */


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

I did see that it was auto-prefixing some styles, but the input placeholder one unfortunately wasn't being done for some reason.

jelbourn · 2017-03-20T19:27:10Z

src/app/shared/navbar/searchbar/searchbar.ts

+import {Router} from '@angular/router';
+import {FormControl} from '@angular/forms';
+import {MdSnackBar} from '@angular/material';
+import {Observable} from 'rxjs';


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.

jelbourn · 2017-03-20T19:27:45Z

src/assets/img/icons/ic_search_white_24px.svg

+<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>


Is there a font icon for search that would serve for this instead of using an svg?

riavalon · 2017-06-05T19:46:21Z

@jelbourn This should be ready for another look over again

jelbourn · 2017-06-06T18:33:25Z

src/app/shared/navbar/searchbar/searchbar.html

+    placeholder="Search"
+    type="text"
+    (focus)="toggleIsExpanded(true)"
+    (blur)="toggleIsExpanded(false)"


This method doesn't take an argument

jelbourn · 2017-06-06T18:35:51Z

src/app/shared/navbar/searchbar/searchbar.scss

@@ -0,0 +1,60 @@
+$input-bg: #85D9E2;


All of the colors should be part of the theme

jelbourn · 2017-06-06T18:36:36Z

src/app/shared/navbar/searchbar/searchbar.html

+    (keyup.enter)="handlePlainSearch($event.target.value.toLowerCase())"
+    [mdAutocomplete]="auto"
+    [formControl]="searchControl">
+  <i class="material-icons">search</i>


This should probably be <md-icon>

jelbourn · 2017-06-06T18:38:59Z

src/app/shared/navbar/searchbar/searchbar.scss

+    }
+  }
+
+  input {


Can this be done with a css class instead? E.g. .docs-search-input

jelbourn · 2017-06-06T18:41:12Z

src/app/shared/navbar/searchbar/searchbar.ts

+  }
+
+  public handlePlainSearch(searchTerm) {
+    const item = this.allDocItems.find(item => item.name.toLowerCase() === searchTerm);


Partial matches should also jump to the first result. E.g., typing "Date" and hitting enter should jump to the datepicker.

jelbourn · 2017-06-06T18:41:29Z

src/app/shared/navbar/searchbar/searchbar.ts

+
+  public handlePlainSearch(searchTerm) {
+    const item = this.allDocItems.find(item => item.name.toLowerCase() === searchTerm);
+    item ? this.navigate(item.id) : this._showError();


We should clear the input on a successful search

jelbourn · 2017-06-06T18:42:40Z

src/app/shared/navbar/searchbar/searchbar.ts

+  @ViewChild(MdAutocompleteTrigger)
+  private _autocompleteTrigger: MdAutocompleteTrigger;
+
+  public allDocItems: DocItem[];


Don't need to explicitly say public since that's the default

jelbourn · 2017-06-06T18:43:19Z

src/app/shared/navbar/searchbar/searchbar.ts

+  public allDocItems: DocItem[];
+  public filteredSuggestions: Observable<DocItem[]>;
+  public searchControl: FormControl = new FormControl('');
+  public sub;


sub should have a more specific name and probably a type

riavalon · 2017-06-07T16:45:31Z

@jelbourn Updated this with the recent requested changes

jelbourn · 2017-06-07T20:37:20Z

Looks good except for one last thing I noticed:

When the input is first focused, the autocomplete uses the original width before the input finishes expanding. In reality this is probably a while new feature we need in autocomplete, but for now I think we can work around it by immediately closing the autocomplete on focus and then opening it manually when the input's expansion animation completes.

riavalon · 2017-06-09T18:06:52Z

@jelbourn Set the panel to open and close in sync with the expanding animation

jelbourn · 2017-06-16T00:19:54Z

@riavalon looks like clicking on the autocomplete items doesn't navigate any more

riavalon · 2017-06-16T15:47:07Z

@jelbourn Blur event was being called when trying to click on an option. I rewrote the focus/blur logic to ignore clicks on md-options.

jotatoledo · 2018-07-31T10:40:42Z

Up, this would be a nice feature on the docs site

googlebot added the cla: yes label Mar 17, 2017

riavalon closed this Mar 17, 2017

riavalon reopened this Mar 17, 2017

jelbourn requested review from jelbourn and kara March 20, 2017 18:33

jelbourn assigned jelbourn and kara Mar 20, 2017

kara reviewed Mar 20, 2017

View reviewed changes

kara assigned riavalon and unassigned jelbourn and kara Mar 20, 2017

jelbourn reviewed Mar 20, 2017

View reviewed changes

riavalon force-pushed the feat-search-bar branch from 91044c5 to f0507bf Compare March 22, 2017 18:39

riavalon mentioned this pull request Mar 24, 2017

feat(navbar): Add themepicker component with lazy loaded themes #136

Merged

riavalon force-pushed the feat-search-bar branch from f0507bf to fe77301 Compare March 30, 2017 18:00

riavalon force-pushed the feat-search-bar branch 4 times, most recently from 87a943c to a49b4ce Compare April 13, 2017 17:23

riavalon force-pushed the feat-search-bar branch 5 times, most recently from c1c7f81 to f688a90 Compare April 27, 2017 18:22

riavalon force-pushed the feat-search-bar branch from f688a90 to cdb7538 Compare June 5, 2017 19:40

jelbourn reviewed Jun 6, 2017

View reviewed changes

riavalon force-pushed the feat-search-bar branch from cdb7538 to aa292f6 Compare June 7, 2017 15:55

riavalon force-pushed the feat-search-bar branch from aa292f6 to 0c6c522 Compare June 9, 2017 18:05

riavalon force-pushed the feat-search-bar branch from 0c6c522 to 7ffa889 Compare June 15, 2017 19:06

riavalon force-pushed the feat-search-bar branch from 7ffa889 to a3166ea Compare June 16, 2017 15:46

riavalon force-pushed the feat-search-bar branch from a3166ea to 2fcf039 Compare June 22, 2017 17:01

feat(navbar): Add basic searchbar component to site.

dfcf761

riavalon force-pushed the feat-search-bar branch from 2fcf039 to dfcf761 Compare June 26, 2017 14:51

EdricChan03 mentioned this pull request Jul 31, 2018

[feature request] add search box in material.angular.io angular/components#12446

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(navbar): Add basic search bar component to site. #134

feat(navbar): Add basic search bar component to site. #134

riavalon commented Mar 17, 2017 •

edited

Loading

kara left a comment

kara Mar 20, 2017

riavalon Mar 21, 2017

kara Mar 21, 2017

riavalon Mar 22, 2017

kara Mar 20, 2017

riavalon Mar 20, 2017

kara Mar 20, 2017

riavalon Mar 21, 2017

kara Mar 20, 2017 •

edited

Loading

kara Mar 20, 2017 •

edited

Loading

kara Mar 20, 2017

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

riavalon Mar 20, 2017

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

riavalon commented Jun 5, 2017

jelbourn Jun 6, 2017

jelbourn Jun 6, 2017

jelbourn Jun 6, 2017

jelbourn Jun 6, 2017

jelbourn Jun 6, 2017 •

edited

Loading

jelbourn Jun 6, 2017

jelbourn Jun 6, 2017

jelbourn Jun 6, 2017

riavalon commented Jun 7, 2017

jelbourn commented Jun 7, 2017

riavalon commented Jun 9, 2017

jelbourn commented Jun 16, 2017

riavalon commented Jun 16, 2017

jotatoledo commented Jul 31, 2018

+                  }
+                }
+                input {

feat(navbar): Add basic search bar component to site. #134

Are you sure you want to change the base?

feat(navbar): Add basic search bar component to site. #134

Conversation

riavalon commented Mar 17, 2017 • edited Loading

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kara Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

kara Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riavalon commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riavalon commented Jun 7, 2017

jelbourn commented Jun 7, 2017

riavalon commented Jun 9, 2017

jelbourn commented Jun 16, 2017

riavalon commented Jun 16, 2017

jotatoledo commented Jul 31, 2018

riavalon commented Mar 17, 2017 •

edited

Loading

kara Mar 20, 2017 •

edited

Loading

kara Mar 20, 2017 •

edited

Loading

jelbourn Jun 6, 2017 •

edited

Loading