Skip to content
This repository has been archived by the owner on Jul 6, 2020. It is now read-only.

Custom directive added for authChecking #302

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Kajol-Kumari
Copy link
Member

@Kajol-Kumari Kajol-Kumari commented Mar 16, 2020

work done in this PR:

All the logic goes in directive and code becomes more cleaner.

It is implementation of one of the idea introduced under GSOC'20 Idea List

GIF showing functionality working fine after doing the changes:
ezgif com-video-to-gif (2)

@codecov-io
Copy link

codecov-io commented Mar 18, 2020

Codecov Report

Merging #302 into master will increase coverage by 0.18%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #302      +/-   ##
==========================================
+ Coverage   50.62%   50.80%   +0.18%     
==========================================
  Files          66       67       +1     
  Lines        3771     3785      +14     
  Branches      444      445       +1     
==========================================
+ Hits         1909     1923      +14     
+ Misses       1767     1766       -1     
- Partials       95       96       +1     
Impacted Files Coverage Δ
src/app/Directives/authcheck.directive.ts 92.85% <92.85%> (ø)
...bliclists/challengelist/challengelist.component.ts 47.57% <0.00%> (+0.97%) ⬆️
Impacted Files Coverage Δ
src/app/Directives/authcheck.directive.ts 92.85% <92.85%> (ø)
...bliclists/challengelist/challengelist.component.ts 47.57% <0.00%> (+0.97%) ⬆️

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 d3d23ca...9a75649. Read the comment docs.

@RishabhJain2018
Copy link
Member

@Sanji515 Can you please review this?

Copy link
Member

@Shekharrajak Shekharrajak left a comment

Choose a reason for hiding this comment

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

This is looking good and I am expecting similar directives to be implemented.

But think to make it modular: what if we want to have multiple conditions - as input for the directives ?

import { AuthService } from '../services/auth.service';

@Directive({
selector: '[appAuthcheck]'
Copy link
Member

Choose a reason for hiding this comment

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

better selector name - a generic one

private viewContainer: ViewContainerRef
) { }

condition: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

variable name should define the value it contains.
Try to make a input value for the directive.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2020

Codecov Report

Merging #302 into master will increase coverage by 0.15%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #302      +/-   ##
==========================================
+ Coverage   50.16%   50.32%   +0.15%     
==========================================
  Files          66       67       +1     
  Lines        3873     3887      +14     
  Branches      450      451       +1     
==========================================
+ Hits         1943     1956      +13     
  Misses       1836     1836              
- Partials       94       95       +1     
Impacted Files Coverage Δ
src/app/Directives/authcheck.directive.ts 92.85% <92.85%> (ø)
Impacted Files Coverage Δ
src/app/Directives/authcheck.directive.ts 92.85% <92.85%> (ø)

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 932a9c0...5a8083f. Read the comment docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants