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

Added search suggestion capabilities for nbsearch #1038

Merged
merged 21 commits into from
Apr 11, 2019
Merged

Added search suggestion capabilities for nbsearch #1038

merged 21 commits into from
Apr 11, 2019

Conversation

WLun001
Copy link
Contributor

@WLun001 WLun001 commented Dec 3, 2018

input event will trigger on every keystroke, therefore other developer can handle it and provide search suggestion as the user is typing.

Demo on http://localhost:4200/#/search/search-event.component

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

Added search suggestion capabilities, that will emit event on every keystroke.
This can be handle by developers like this: https://angular.io/tutorial/toh-pt6#search-by-name

`input` event will trigger on every keystroke, therefore other developer can handle it and provide search suggestion as the user is typing.
@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #1038 into master will increase coverage by 0.67%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1038      +/-   ##
==========================================
+ Coverage   81.16%   81.84%   +0.67%     
==========================================
  Files         232      232              
  Lines        7057     7067      +10     
  Branches      599      599              
==========================================
+ Hits         5728     5784      +56     
+ Misses       1153     1104      -49     
- Partials      176      179       +3
Impacted Files Coverage Δ
...ramework/theme/components/search/search.service.ts 84% <100%> (+29%) ⬆️
...mework/theme/components/search/search.component.ts 79.48% <100%> (+33.05%) ⬆️
...mework/theme/components/layout/layout.component.ts 75.7% <0%> (+1.69%) ⬆️
src/framework/theme/services/theme.service.ts 97.61% <0%> (+4.76%) ⬆️

@nnixaa nnixaa requested a review from Tibing December 13, 2018 16:41
Copy link
Contributor

@yggg yggg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @WLun001. Thanks a lot for the PR!

Let's add a new @Output input for input event in NbSearchFieldComponent instead of changing submitSearch behavior. This way we wouldn't break search API and users won't have to change their code after updating.

Also, please revert package-lock.json changes.

@WLun001
Copy link
Contributor Author

WLun001 commented Dec 24, 2018

Hi @yggg

Do you have any idea where should the search suggestion to be displayed if other developer used it?
Currently the animated searches are taking whole screen, if the suggestion appears on their page (for example, layout rotate search will push their page downwards), would be kinda weird.

@yggg
Copy link
Contributor

yggg commented Jan 8, 2019

Hi @WLun001. Unfortunately, there is no way to customize search screen.
As a workaround, you can create custom search components by extending NbSearchFieldComponent and NbSearchComponent and copying their templates. Then modify NbSearchFieldComponent template to have not only input but also search suggestions. Something like this:

@Component({
  selector: 'custom-search-field',
  // copied and modified template of NbSearchFieldComponent
  template: `
    ...
      <input #searchInput />
      <ul #options>...</ul>
    ...
  `,
})
class CustomSearchFieldComponent extends NbSearchFieldComponent {}

@Component({
  selector: 'custom-search-field',
  // template of NbSearchComponent. replace nb-search-field with custom-search-field
  template: `...`,
})
class CustomSearchComponentComponent extends NbSearchComponent {}

Copy link
Contributor

@yggg yggg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WLun001 Thanks for addressing review comments. Please, take a look at new ones. There are just a few left.
And thank you a lot for docs example as well!

@yggg yggg removed the request for review from Tibing March 6, 2019 15:48
yggg
yggg previously approved these changes Mar 6, 2019
@yggg yggg requested review from nnixaa and yggg and removed request for yggg April 11, 2019 14:00
</nb-card>
</nb-layout-column>
</nb-layout>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary empty line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in f5d96dc

@yggg yggg merged commit 58fa556 into akveo:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants