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.

Sign up for GitHub

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

Jump to bottom

Refactors Theme Component and increase coverage #1340

Merged
merged 1 commit into from
Jan 21, 2019
Merged

Refactors Theme Component and increase coverage #1340

merged 1 commit into from
Jan 21, 2019

Conversation

shreyanshdwivedi
Copy link
Member

Fixes #1339

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:

  • Refactored code
  • Added tests to increase coverage

@shreyanshdwivedi shreyanshdwivedi mentioned this pull request Dec 23, 2018
Merged
4 tasks
@shreyanshdwivedi
Copy link
Member Author

@praveenojha33 @AakashMallik @simsausaurabh we may need to discuss how to tackle the error in travis. This is a well know error which occurs when we test our function.
Several issues were opened in angular-cli org for this, for reference angular/angular-cli#7296

@praveenojha33
Copy link
Member

praveenojha33 commented Dec 26, 2018 •
edited
Loading

@shreyanshdwivedi That issue has been fixed (as mentioned in the comments there). There are some merged PRs also which has referenced that issue. We can look into that and see what changes they have made.
If then also this issue occurs please open an issue there or search/ask on stackoverflow 👍

@shreyanshdwivedi shreyanshdwivedi force-pushed the refactorTheme branch 2 times, most recently from 09868d1 to 3008f47 Compare December 26, 2018 11:25
@codecov
Copy link

codecov bot commented Dec 26, 2018 •
edited
Loading

Codecov Report

Merging #1340 into development will increase coverage by 5.13%.
The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1340      +/-   ##
===============================================
+ Coverage        55.64%   60.77%   +5.13%     
===============================================
  Files               51       51              
  Lines             1436     1438       +2     
  Branches           178      180       +2     
===============================================
+ Hits               799      874      +75     
+ Misses             532      455      -77     
- Partials           105      109       +4
Impacted Files Coverage Δ
src/app/theme/theme.component.ts 90.52% <0%> (+76.54%) ⬆️
src/app/services/intelligence.service.ts 79.16% <0%> (+8.33%) ⬆️

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 1b2fc9f...09540f4. Read the comment docs.

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Dec 26, 2018 •
edited
Loading

@praveenojha33 @simsausaurabh Please review. :)
FYI the issue I was facing was that real problem was not shown in error log, but using flag --sm=false we can get insight to the real problem.

praveenojha33
praveenojha33 requested changes Dec 30, 2018
View reviewed changes
Copy link
Member

@praveenojha33 praveenojha33 left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts.

@shreyanshdwivedi
Copy link
Member Author

@praveenojha33 which conflicts? My branch is showing no conflict with base branch

@shreyanshdwivedi
Copy link
Member Author

@simsausaurabh @praveenojha33 please review

@praveenojha33
Copy link
Member

There are conflicts due to which this PR cannot be merged.
image Please remove conflicts.

sakshee-19
sakshee-19 reviewed Jan 6, 2019
View reviewed changes
Copy link
Contributor

@sakshee-19 sakshee-19 left a comment

Choose a reason for hiding this comment

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

Is it fine to use then?

src/app/theme/theme.component.spec.ts
btn.click();

fixture.detectChanges();
fixture.whenStable().then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok to use then? Can't it be done without it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually whatever reference I got about whenStable with async, I found the same format. I don't think it'll work without this

@shreyanshdwivedi
Copy link
Member Author

@praveenojha33 please review now and let me know if the conflicts still exists

praveenojha33
praveenojha33 approved these changes Jan 14, 2019
View reviewed changes
Copy link
Member

@praveenojha33 praveenojha33 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@praveenojha33 praveenojha33 merged commit c6de390 into fossasia:development Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@praveenojha33 praveenojha33 praveenojha33 approved these changes

@sakshee-19 sakshee-19 sakshee-19 approved these changes

@simsausaurabh simsausaurabh simsausaurabh approved these changes

@bhaveshAn bhaveshAn Awaiting requested review from bhaveshAn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shreyanshdwivedi @praveenojha33 @simsausaurabh @sakshee-19