Allow for multiple classes #1842

HJain01 · 2018-07-19T14:16:30Z

This allows for multiple classes to be input into sky-icon

blackbaud-sky-savage · 2018-07-19T14:38:58Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: cdecb5a
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/405830355

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2018-07-19T15:03:25Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 55fde1a
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/405840078

(Please note that this is a fully automated comment.)

…into sky-icon-edits

blackbaud-sky-savage · 2018-07-19T17:56:46Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: f7a497f
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/405913126

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2018-07-19T18:54:09Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 1f2b52f
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/405931993

(Please note that this is a fully automated comment.)

codecov-io · 2018-07-19T18:54:44Z

Codecov Report

Merging #1842 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1842      +/-   ##
==========================================
+ Coverage   99.98%   99.98%   +<.01%     
==========================================
  Files         410      412       +2     
  Lines        8592     8610      +18     
  Branches     1269     1271       +2     
==========================================
+ Hits         8591     8609      +18     
  Misses          1        1

Impacted Files	Coverage Δ
src/modules/icon/icon.component.ts	`100% <100%> (ø)`
src/modules/icon/icon.module.ts	`100% <100%> (ø)`

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 ab24902...39ed79c. Read the comment docs.

blackbaud-sky-savage · 2018-07-19T19:28:52Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 257b8e4
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/405948527

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2018-07-19T19:37:28Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 7ea881c
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/405948616

(Please note that this is a fully automated comment.)

blackbaud-conorwright · 2018-07-19T19:52:08Z

skyux-spa-visual-tests/src/app/icon/icon-visual.component.html

@@ -0,0 +1,12 @@
+<div id="screenshot-icon">


Style comment:
attributes go on their own lines for all html tags.

blackbaud-conorwright · 2018-07-19T19:54:49Z

src/demos/icon/icon-demo.component.html

@@ -0,0 +1,10 @@
+<sky-icon icon="spinner" size="3x" fixedWidth="true"></sky-icon>


attributes on different lines in alphabetical order

blackbaud-conorwright · 2018-07-19T19:55:16Z

src/modules/icon/fixtures/icon.component.fixture.html

@@ -0,0 +1 @@
+<sky-icon [icon]="icon" [size]="size" [fixedWidth]="fixedWidth"></sky-icon>


attributes on different lines in alphabetical order

blackbaud-conorwright · 2018-07-19T19:58:32Z

src/modules/icon/icon.component.html

@@ -0,0 +1,5 @@
+<i *ngIf="icon"


move *ngIf to its own line

blackbaud-conorwright · 2018-07-19T20:02:33Z

src/modules/icon/icon.component.spec.ts

+    expect(element.querySelector('.sky-icon').classList.length).toBe(4);
+  });
+
+  it('should display something other than circle', () => {


I would rename this to say that it is testing that an icon is displayed with a fixed width and no size

blackbaud-conorwright · 2018-07-19T20:06:43Z

src/modules/icon/icon.component.spec.ts

+
+  it('should display something other than circle', () => {
+    cmp.icon = 'broom';
+    cmp.size = '';


use undefined instead of empty string to represent the value not being provided

blackbaud-conorwright · 2018-07-19T20:09:40Z

src/modules/icon/icon.component.ts

+  public size: string;
+
+  @Input()
+  public fixedWidth: boolean = true;


Do we want this to default to false?

blackbaud-conorwright · 2018-07-19T20:10:30Z

src/modules/icon/icon.component.ts

+    if (this.size !== '') {
+      list.push('fa-' + this.size);
+    }
+    if (this.fixedWidth === true) {


since fixedWidth is a boolean, this could just be
if (this.fixedWidth) {

blackbaud-conorwright · 2018-07-19T20:12:37Z

src/modules/icon/icon.component.ts

+  public classList(): string[] {
+    let list: string[] = [];
+    list.push('fa-' + this.icon);
+    if (this.size !== '') {


In javascript all values are truthy or falsy and in this case we will want to cover the scenarios where size is undefined or ''
Both of those are considered falsy. So this can be:
if (this.size) {

and it will cover both cases

blackbaud-conorwright

just two small things

blackbaud-conorwright · 2018-07-19T20:26:37Z

src/modules/icon/icon.component.spec.ts

@@ -15,7 +15,7 @@ import {
  expect
 } from '@blackbaud/skyux-builder/runtime/testing/browser';

-describe('Icon component', () => {
+fdescribe('Icon component', () => {


missed an "f" 😉

blackbaud-conorwright · 2018-07-19T20:27:24Z

src/modules/icon/fixtures/icon.component.fixture.html

+<sky-icon
+  [icon]="icon"
+  [size]="size"
+  [fixedWidth]="fixedWidth">


Alphabetical
<sky-icon
[fixedWidth]="fixedWidth"
[icon]="icon"
[size]="size">

blackbaud-sky-savage · 2018-07-19T20:38:02Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: da17078
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/405976464

(Please note that this is a fully automated comment.)

blackbaud-conorwright

Looks great, Harsh! 😃

blackbaud-sky-savage · 2018-07-20T12:32:46Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 2fbcc76
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/406216342

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2018-07-20T12:51:42Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 39ed79c
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/406218012

(Please note that this is a fully automated comment.)

blackbaud-conorwright · 2018-07-20T16:46:15Z

closing in favor of #1846
Thank you for contributing!

Harsh Jain and others added 20 commits July 13, 2018 14:49

sky-icon component

1932ea5

finishing up component

2c37daa

created demo for sky-icon component

534682e

added imports and exports to core, demo, and index

4792244

Added the visual test

9b898bf

Fixing unit tests

b58160d

Fixing TSLint issues

57eeff6

Fixed visual test

581ddcb

updating visual test

752c8cb

fixing TSLint errors

26a7c87

fdescribe to describe

16cd7aa

Merge branch 'master' into sky-icon

0d3384e

fixing merge

7561296

Merge branch 'master' into sky-icon

c8abd9a

Merged master

12eba05

Fixed missing tile style sheet

a2422f9

Updated demo

ac41d78

Added more icons to visual test

325bfad

Merge branch 'master' into contrib-sky-icon

3072ee8

Allow for multiple classes

cdecb5a

Merge branch 'master' into sky-icon-edits

55fde1a

Harsh Jain added 2 commits July 19, 2018 13:34

Fixed unit test

beec5db

Merge branch 'sky-icon-edits' of https://github.com/Remulus2006/skyux2 …

f7a497f

…into sky-icon-edits

Added unit test to check for optional fixedWidth

1f2b52f

Checks to see how many classes there are

7ea881c

fdescribe to describe

257b8e4

blackbaud-conorwright reviewed Jul 19, 2018

View reviewed changes

src/modules/icon/icon.component.html Outdated

@@ -0,0 +1,5 @@

<i *ngIf="icon"

Copy link

Contributor

blackbaud-conorwright Jul 19, 2018

Choose a reason for hiding this comment

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

move *ngIf to its own line

blackbaud-conorwright reviewed Jul 19, 2018

View reviewed changes

blackbaud-conorwright suggested changes Jul 19, 2018

View reviewed changes

Fixed PR suggestions

da17078

blackbaud-conorwright suggested changes Jul 19, 2018

View reviewed changes

Harsh Jain added 2 commits July 20, 2018 08:10

changed to alphabetical order and got rid of an f

2fbcc76

Moved angled bracket

39ed79c

blackbaud-conorwright approved these changes Jul 20, 2018

View reviewed changes

This was referenced Jul 20, 2018

Icon component #1834

Closed

Contrib sky icon edits #1846

Merged

blackbaud-conorwright closed this Jul 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for multiple classes #1842

Allow for multiple classes #1842

HJain01 commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

codecov-io commented Jul 19, 2018 •

edited

Loading

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright left a comment

blackbaud-conorwright Jul 19, 2018

blackbaud-conorwright Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-conorwright left a comment

blackbaud-sky-savage commented Jul 20, 2018

blackbaud-sky-savage commented Jul 20, 2018

blackbaud-conorwright commented Jul 20, 2018

		@@ -0,0 +1,10 @@
		<sky-icon icon="spinner" size="3x" fixedWidth="true"></sky-icon>

		@@ -0,0 +1 @@
		<sky-icon [icon]="icon" [size]="size" [fixedWidth]="fixedWidth"></sky-icon>

Allow for multiple classes #1842

Allow for multiple classes #1842

Conversation

HJain01 commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

codecov-io commented Jul 19, 2018 • edited Loading

Codecov Report

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-sky-savage commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackbaud-conorwright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackbaud-sky-savage commented Jul 19, 2018

blackbaud-conorwright left a comment

Choose a reason for hiding this comment

blackbaud-sky-savage commented Jul 20, 2018

blackbaud-sky-savage commented Jul 20, 2018

blackbaud-conorwright commented Jul 20, 2018

codecov-io commented Jul 19, 2018 •

edited

Loading