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(navbar): Add basic search bar component to site. #134

Open
wants to merge 1 commit into
base: master
from

Conversation

@riavalon
Copy link
Contributor

@riavalon riavalon commented Mar 17, 2017

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

@googlebot googlebot added the cla: yes label Mar 17, 2017
@riavalon riavalon closed this Mar 17, 2017
@riavalon riavalon reopened this Mar 17, 2017
@jelbourn jelbourn requested review from jelbourn and kara Mar 20, 2017
Copy link
Contributor

@kara kara left a comment

Can you also add some tests? :D

}

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

This comment has been minimized.

@kara

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

This comment has been minimized.

@riavalon

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

This comment has been minimized.

@kara

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.

This comment has been minimized.

@riavalon

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.

})

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

This comment has been minimized.

@kara

kara Mar 20, 2017
Contributor

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

This comment has been minimized.

@riavalon

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

This comment has been minimized.

@kara

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)

This comment has been minimized.

@riavalon

riavalon Mar 21, 2017
Author Contributor

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

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

This comment has been minimized.

@kara

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

</div>

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

This comment has been minimized.

@kara

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.

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

This comment has been minimized.

@kara

kara Mar 20, 2017
Contributor

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

This comment has been minimized.

@jelbourn

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.

@kara kara assigned riavalon and unassigned jelbourn and kara Mar 20, 2017
box-sizing: border-box;
}

&.expanded .search-input-container {

This comment has been minimized.

@jelbourn

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)

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

This comment has been minimized.

@jelbourn

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

This comment has been minimized.

@riavalon

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.

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

This comment has been minimized.

@jelbourn

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.

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

This comment has been minimized.

@jelbourn

jelbourn Mar 20, 2017
Member

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

@riavalon riavalon force-pushed the riavalon:feat-search-bar branch from 91044c5 to f0507bf Mar 22, 2017
@riavalon riavalon force-pushed the riavalon:feat-search-bar branch from f0507bf to fe77301 Mar 30, 2017
@riavalon riavalon force-pushed the riavalon:feat-search-bar branch 4 times, most recently from 87a943c to a49b4ce Apr 13, 2017
@riavalon riavalon force-pushed the riavalon:feat-search-bar branch 5 times, most recently from c1c7f81 to f688a90 Apr 26, 2017
@riavalon riavalon force-pushed the riavalon:feat-search-bar branch from f688a90 to cdb7538 Jun 5, 2017
@riavalon
Copy link
Contributor Author

@riavalon riavalon commented Jun 5, 2017

@jelbourn This should be ready for another look over again

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

This comment has been minimized.

@jelbourn

jelbourn Jun 6, 2017
Member

This method doesn't take an argument

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

This comment has been minimized.

@jelbourn

jelbourn Jun 6, 2017
Member

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>

This comment has been minimized.

@jelbourn

jelbourn Jun 6, 2017
Member

This should probably be <md-icon>

}
}

input {

This comment has been minimized.

@jelbourn

jelbourn Jun 6, 2017
Member

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

This comment has been minimized.

@jelbourn

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.


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

This comment has been minimized.

@jelbourn

jelbourn Jun 6, 2017
Member

We should clear the input on a successful search

@ViewChild(MdAutocompleteTrigger)
private _autocompleteTrigger: MdAutocompleteTrigger;

public allDocItems: DocItem[];

This comment has been minimized.

@jelbourn

jelbourn Jun 6, 2017
Member

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;

This comment has been minimized.

@jelbourn

jelbourn Jun 6, 2017
Member

sub should have a more specific name and probably a type

@riavalon riavalon force-pushed the riavalon:feat-search-bar branch from cdb7538 to aa292f6 Jun 7, 2017
@riavalon
Copy link
Contributor Author

@riavalon riavalon commented Jun 7, 2017

@jelbourn Updated this with the recent requested changes

@jelbourn
Copy link
Member

@jelbourn jelbourn commented Jun 7, 2017

Looks good except for one last thing I noticed:
image

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 riavalon force-pushed the riavalon:feat-search-bar branch from aa292f6 to 0c6c522 Jun 9, 2017
@riavalon
Copy link
Contributor Author

@riavalon riavalon commented Jun 9, 2017

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

@riavalon riavalon force-pushed the riavalon:feat-search-bar branch from 0c6c522 to 7ffa889 Jun 15, 2017
@jelbourn
Copy link
Member

@jelbourn jelbourn commented Jun 16, 2017

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

@riavalon riavalon force-pushed the riavalon:feat-search-bar branch from 7ffa889 to a3166ea Jun 16, 2017
@riavalon
Copy link
Contributor Author

@riavalon riavalon commented Jun 16, 2017

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

@riavalon riavalon force-pushed the riavalon:feat-search-bar branch from a3166ea to 2fcf039 Jun 22, 2017
@jotatoledo
Copy link

@jotatoledo jotatoledo commented Jul 31, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.