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

Fixed clear search issue #1230 #1343

Merged
merged 1 commit into from
Dec 27, 2018

Conversation

reeversedev
Copy link
Contributor

@reeversedev reeversedev commented Dec 23, 2018

Fixes #1230

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream master branch.
  • I have added necessary documentation (if appropriate)
  • Added Surge preview link

Changes proposed in this pull request:

  • Clears the input after clicking the logo on navbar.

@codecov
Copy link

codecov bot commented Dec 23, 2018

Codecov Report

Merging #1343 into development will decrease coverage by 0.02%.
The diff coverage is 55.55%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1343      +/-   ##
===============================================
- Coverage        54.89%   54.87%   -0.03%     
===============================================
  Files               51       51              
  Lines             1419     1425       +6     
  Branches           176      177       +1     
===============================================
+ Hits               779      782       +3     
- Misses             534      536       +2     
- Partials           106      107       +1
Impacted Files Coverage Δ
src/app/search-bar/search-bar.component.ts 54.23% <50%> (-0.15%) ⬇️
src/app/navbar/navbar.component.ts 70.58% <57.14%> (-6.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4cf69e...6fe943f. Read the comment docs.

@reeversedev
Copy link
Contributor Author

@simsausaurabh @mariobehling Please review. Thanks.

Copy link
Member

@simsausaurabh simsausaurabh left a comment

Choose a reason for hiding this comment

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

Please see inline

} from '@angular/core';
import { ActivatedRoute, Router } from '@angular/router';
import { Store } from '@ngrx/store';
import * as fromRoot from '../reducers';
Copy link
Member

Choose a reason for hiding this comment

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

Why changing to double quotes? Its standard used in project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah! I just checked. Was a problem with my vscode. Just fixed it and pushed the code. Can you please check and review again?

Copy link
Member

@shreyanshdwivedi shreyanshdwivedi left a comment

Choose a reason for hiding this comment

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

Please squash your commits.

Fixed double quotes to single quotes

Fixed reference for navbar html

Fixed spacing issues in constructor scope

Fixed clear search issue fossasia#1230
@reeversedev
Copy link
Contributor Author

@shreyanshdwivedi Just squashed the commits. Are we good to go?

Copy link
Member

@shreyanshdwivedi shreyanshdwivedi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@shreyanshdwivedi
Copy link
Member

@reeversedev I'll suggest you to add proper test for the function you added. We are working to increase the coverage of every component and your PR decreases coverage of navbar component by 6%
If you want, you can open a separate issue to refactor and increase coverage of navbar component.

@reeversedev
Copy link
Contributor Author

@shreyanshdwivedi Alright. I'll create a separate issue for that. Till that can you put this in WIP tag?

@shreyanshdwivedi
Copy link
Member

@reeversedev we can proceed by merging this if maintainers are okay with your pull request. Afterwards, you can create an issue and work on that and create a separate PR for refactoring and increasing code coverage :)

@reeversedev
Copy link
Contributor Author

@shreyanshdwivedi Yeah. Let's see if mentors are good to go with this.

@praveenojha33 praveenojha33 merged commit a0d1a93 into fossasia:development Dec 27, 2018
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.

None yet

6 participants