-
Notifications
You must be signed in to change notification settings - Fork 65
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2013 +/- ##
==========================================
+ Coverage 99.94% 99.94% +<.01%
==========================================
Files 418 418
Lines 8919 8939 +20
Branches 1326 1334 +8
==========================================
+ Hits 8914 8934 +20
Misses 5 5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments for you. Otherwise looks good!
<div *ngIf="!isHorizontal || isActive" | ||
class="sky-progress-indicator-item-heading" | ||
[ngClass]="{ | ||
'sky-text-success': isComplete && !isActive && !isHorizontal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ngClass
has an awful lot of conditionals - should we move this to the component so it can be covered with tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
</div> | ||
</div> | ||
<div *ngIf="!isHorizontal || isActive" | ||
<div *ngIf="!(isHorizontal || isPassive) || isPassive && !isLastItem || isActive" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here. We have a lot of logic in both the <div>
and the ngClass
. Should the logic be moved to our component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
> | ||
</sky-progress-indicator-item> | ||
<sky-progress-indicator-item | ||
title="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this last step show some sort of text? Since this is a demo, I'm wondering if might mislead consumers and make them think a piece of programming is necessary to make this text show.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I put in "HIDDEN TITLE". That way if they look at the code it may be more clear.
Implemented a passive progress indicator mode
Fixed sizing of progress indicator steps
Resolves: #2004