feat(operators): add searchbar #264
Conversation
Codecov Report
@@ Coverage Diff @@
## master #264 +/- ##
=========================================
Coverage ? 79.64%
=========================================
Files ? 17
Lines ? 226
Branches ? 10
=========================================
Hits ? 180
Misses ? 44
Partials ? 2
Continue to review full report at Codecov.
|
|
It looks like there is a merge conflict here. Can you resolve that? |
|
thank you for your review. To be honest I would have expected several issues, but I'm totally fine with it :D |
| withLatestFrom, | ||
| flatMap, | ||
| map, | ||
| merge, |
There was a problem hiding this comment.
There are imports here which are unused.
|
@jwo719 Also, while you filter down there is an empty space created for each of the category group that doesn't have any operators for the filtered value.
|
|
Just a style preference, but I would consider removing the floating placeholder for the search bar. I'm also not sure if we need the bottom line or if we could use the below header to frame it. What do you all think?
|
| <mat-nav-list *ngFor="let category of categories" | ||
| class="operator-list"> | ||
| <h3 mat-subheader | ||
| class="category-subheader">{{ category }}</h3> | ||
| <h3 mat-subheader class="category-subheader" *ngIf="(groupedOperators[category] | filter: searchInput.value).length > 0">{{ category }}</h3> |
There was a problem hiding this comment.
This wont have a good performance as the # of operators increase, as the filtering is run 2 times. Wouldn't it be better to make the collection of operators mapped to category reactive?
Idea:
Build from Map<category => operator[]> a Map<category => Observable<operator[]>>
There was a problem hiding this comment.
yeah I had that idea too, but last time I was simply to supid to implement it that way. But you are definetly right, I will give it another try :D
| if (!items || !filter) { | ||
| return items; | ||
| } | ||
| return items.filter(item => item.name.indexOf(filter) !== -1); |
There was a problem hiding this comment.
I don´t like the pipe approach for this case.
Having no type is a two edged blade: doesn't tight the use of the pipe with a concrete type, but you lose type check at compile time.
|
@btroncone agreed on removal of floating placeholder and bottom line. A UX concern I have is that the user is required to delete characters from search in order to reset the list. I think there should be an automatic clearing of the search if an operator is selected, and/or a reset button should be available. Perhaps we could...
|
| @@ -21,6 +21,7 @@ import { WalkthroughComponent } from './components/walkthrough/walkthrough.compo | |||
| import { HighlightJsDirective } from './directives/highlight-js.directive'; | |||
| import { SafeUrlPipe } from './pipes/safe-url.pipe'; | |||
| import { MaterialModule } from '../material/material.module'; | |||
| import { FormsModule, ReactiveFormsModule } from '@angular/forms'; | |||
There was a problem hiding this comment.
I think the import of FormsModule isn't required. At least in the refactored component ngModel hasn't been used.
BTW, could we keep a consistent import style? Maybe the one explained in the angular style guide
| <form> | ||
| <mat-form-field class="full-width-form"> | ||
| <mat-label>Search</mat-label> | ||
| <input matInput value="" [formControl]="searchInput"> |
There was a problem hiding this comment.
Considering that this input is model driven and not template drive, could you move the value initialization of the formControl into the component? IMO is cleaner to not mix template/model driven when possible.
closes #254