-
Notifications
You must be signed in to change notification settings - Fork 65
Vertical tabs #1004
Vertical tabs #1004
Changes from all commits
10f04bd
7beab00
8e11023
76e2f9d
98c645b
c0dc34b
3ae9ce1
574316d
f25ab14
73fc080
ef89910
cf34ef6
507e742
89e5690
b7f30ac
6cf4435
b096cdc
627de29
16dd99a
b13d0d8
b3af3aa
6fb4cdf
a5182ef
4be9bb9
d0ca711
a6d1114
832b7aa
8a66591
c660269
186a2cf
1949bb6
881d319
f98660f
1f2afba
fcf3837
0d72198
9da6441
17704f3
972a83f
5a19a78
40a84f9
491a97b
8b7596d
d6905eb
7bbb099
7cc2f88
08bc38f
06dcd21
9bca2e1
82da583
7d2425c
7a7d127
760de61
a1c1eba
2ad58d6
9bcb3f8
d4610cb
a8c8b1c
2edf339
4fb58bc
ec603a2
8e526ea
20b097f
2a35202
726a523
89b59fa
05f85f7
e0ecb0a
8326139
5191b6c
6b94ca6
403b209
eca97f3
d390a35
128749b
cce1941
147950b
ff0881b
282a7f9
02175f2
77bf128
62903f3
8d07612
f3caf7b
87651ab
d8731c5
f5bf690
0f6562d
e4dbe24
c33acc2
ff431dc
4525f85
7a7d969
8b21d87
128a65e
a0ec1bb
d0e16e3
02b9f3f
a4d5bcf
f7c4b61
c56049f
c02770f
e7f9af3
8d92a5f
77aec8f
d667b19
8600738
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ export class HomeComponent { | |
'text-highlight', | ||
'tiles', | ||
'toolbar', | ||
'vertical-tabs', | ||
'wait' | ||
]; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
<vertical-tabs-visual></vertical-tabs-visual> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
<div id="screenshot-vertical-tabset"> | ||
<sky-vertical-tabset showTabsText='Tab list'> | ||
<sky-vertical-tabset-group | ||
groupHeading="Group 1" | ||
[open]="group1Open" | ||
[disabled]="group1Disabled" | ||
> | ||
<sky-vertical-tab tabHeading="Group 1 Tab 1" tabHeaderCount="5" [active]="active"> | ||
Group 1 Tab 1 content | ||
</sky-vertical-tab> | ||
<sky-vertical-tab tabHeading="Group 1 Tab 2"> | ||
Group 1 Tab 2 content | ||
</sky-vertical-tab> | ||
</sky-vertical-tabset-group> | ||
|
||
<sky-vertical-tabset-group | ||
class="group2" | ||
groupHeading="Group 2" | ||
[open]="group2Open" | ||
[disabled]="group2Disabled" | ||
> | ||
<sky-vertical-tab tabHeading="Group 2 Tab 1" tabHeaderCount="6"> | ||
Group 2 Tab 1 content | ||
</sky-vertical-tab> | ||
<sky-vertical-tab id="group2Tab2" tabHeading="Group 2 Tab 2" tabHeaderCount="0"> | ||
Group 2 Tab 2 content | ||
</sky-vertical-tab> | ||
<sky-vertical-tab | ||
tabHeading="Group 2 Tab 2 - disabled" | ||
tabHeaderCount="0" | ||
[disabled]="tabDisabled" | ||
> | ||
Group 2 Tab 3 content | ||
</sky-vertical-tab> | ||
</sky-vertical-tabset-group> | ||
|
||
<sky-vertical-tabset-group | ||
groupHeading="Group 3" | ||
[open]="group3Open" | ||
[disabled]="group3Disabled" | ||
> | ||
<sky-vertical-tab tabHeading="Group 3 Tab 1"> | ||
Group 3 Tab 1 content | ||
</sky-vertical-tab> | ||
<sky-vertical-tab tabHeading="Group 3 Tab 2"> | ||
Group 3 Tab 2 content | ||
</sky-vertical-tab> | ||
</sky-vertical-tabset-group> | ||
</sky-vertical-tabset> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { ChangeDetectionStrategy, Component} from '@angular/core'; | ||
|
||
@Component({ | ||
selector: 'vertical-tabs-visual', | ||
templateUrl: './vertical-tabs-visual.component.html', | ||
changeDetection: ChangeDetectionStrategy.OnPush | ||
}) | ||
export class VerticalTabsVisualComponent { | ||
public group1Open: boolean = true; | ||
public group1Disabled: boolean = false; | ||
|
||
public group2Open: boolean = false; | ||
public group2Disabled: boolean = false; | ||
|
||
public group3Open: boolean = false; | ||
public group3Disabled: boolean = true; | ||
|
||
public active: boolean = true; | ||
public tabDisabled: boolean = true; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
import { SkyVisualTest } from '../../../config/utils/visual-test-commands'; | ||
import { element, by, browser } from 'protractor'; | ||
|
||
describe('Vertical tabSet', () => { | ||
|
||
it('should match previous vertical tabset screenshot', () => { | ||
return SkyVisualTest.setupTest('vertical-tabs') | ||
.then(() => { | ||
return SkyVisualTest.compareScreenshot({ | ||
screenshotName: 'vertical-tabset', | ||
selector: '#screenshot-vertical-tabset' | ||
}); | ||
}); | ||
}); | ||
|
||
it('should match previous vertical tabset screenshot after clicking tab', () => { | ||
return SkyVisualTest.setupTest('vertical-tabs') | ||
.then(() => { | ||
|
||
const groupElement = element(by.css('.group2')); | ||
browser.wait(function() { return browser.isElementPresent(groupElement); }, 8000); | ||
|
||
// open group | ||
groupElement.click(); | ||
|
||
browser.sleep(1000); | ||
|
||
// click tab | ||
element(by.id('group2Tab2')).click(); | ||
|
||
return SkyVisualTest.compareScreenshot({ | ||
screenshotName: 'vertical-tabset-clicked-tab', | ||
selector: '#screenshot-vertical-tabset' | ||
}); | ||
}); | ||
}); | ||
|
||
it('should match previous vertical tabset screenshot on mobile', () => { | ||
return SkyVisualTest.setupTest('vertical-tabs', 480) | ||
.then(() => { | ||
return SkyVisualTest.compareScreenshot({ | ||
screenshotName: 'vertical-tabset-mobile', | ||
selector: '#screenshot-vertical-tabset' | ||
}); | ||
}); | ||
}); | ||
|
||
it('should match previous vertical tabset screenshot on mobile clicking show tabs', () => { | ||
return SkyVisualTest.setupTest('vertical-tabs', 480) | ||
.then(() => { | ||
|
||
const showTabsButton = element(by.css('.sky-vertical-tabset-show-tabs-btn')); | ||
browser.wait(function() { return browser.isElementPresent(showTabsButton); }, 8000); | ||
|
||
// show tabs | ||
showTabsButton.click(); | ||
|
||
return SkyVisualTest.compareScreenshot({ | ||
screenshotName: 'vertical-tabset-mobile-show-tabs', | ||
selector: '#screenshot-vertical-tabset' | ||
}); | ||
}); | ||
}); | ||
|
||
it('should match previous vertical tabset screenshot on mobile clicking tab', () => { | ||
return SkyVisualTest.setupTest('vertical-tabs', 480) | ||
.then(() => { | ||
|
||
const showTabsButton = element(by.css('.sky-vertical-tabset-show-tabs-btn')); | ||
browser.wait(function() { return browser.isElementPresent(showTabsButton); }, 8000); | ||
|
||
// show tabs | ||
showTabsButton.click(); | ||
browser.sleep(1000); | ||
|
||
// open group | ||
element(by.css('.group2')).click(); | ||
browser.sleep(1000); | ||
|
||
// click tab | ||
element(by.id('group2Tab2')).click(); | ||
|
||
return SkyVisualTest.compareScreenshot({ | ||
screenshotName: 'vertical-tabset-mobile-clicked-tab', | ||
selector: '#screenshot-vertical-tabset' | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
<sky-demo-page title="Vertical tabs"> | ||
<sky-demo-page-summary> | ||
<p> | ||
The vertical tabs module contains components to render a vertical tabset. | ||
</p> | ||
</sky-demo-page-summary> | ||
|
||
<sky-demo-page-properties sectionHeading="Vertical tabset properties"> | ||
<sky-demo-page-property | ||
propertyName="showTabsText" | ||
isOptional="true" | ||
> | ||
Specifies the text to use for the show tabs button in mobile. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd tweak the wording here to "Specifies the text to display on the show tabs button on mobile devices." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that is better. |
||
</sky-demo-page-property> | ||
</sky-demo-page-properties> | ||
|
||
<sky-demo-page-properties sectionHeading="Vertical tab properties"> | ||
<sky-demo-page-property | ||
propertyName="active" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the property names from this point on start with "[" and end with "]"? The properties are in these brackets in the code sample, where as the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In angular, removing the brackets indicates a string literal for the input value. If the brackets are added then it uses object binding and looks for a property on the component with that name. [input]="propertyNameOnComponent" That is angular behavior and I don't think we have to document that. |
||
isOptional="true" | ||
> | ||
Indicates whether or not the tab is active on load | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete "or not" and end with a period. ... I'd also change "on load" to "when the tabset loads." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah agree. |
||
</sky-demo-page-property> | ||
<sky-demo-page-property | ||
propertyName="tabHeading" | ||
> | ||
The text of the button that selects the tab. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems weird to me. I see that the wording here is consistent with the wording for the corresponding property on the tabs module, but referring to this as "the button that selects the tab" seems weird. I mean, in help text, we usually refer to that as the tab. And in the next property, we refer to this as "the tab header." Similarly, in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It definitely doesn't make sense on the vertical tabs to say button, but on the normal tab component, it actually creates buttons that are used to select the tab content. I don't know how we feel about exposing implementation details like that in our docs. I would lean towards making your suggested change in both areas. |
||
</sky-demo-page-property> | ||
<sky-demo-page-property | ||
propertyName="tabHeaderCount" | ||
isOptional="true" | ||
> | ||
Displays a counter alongside the tab header. | ||
</sky-demo-page-property> | ||
<sky-demo-page-property | ||
propertyName="disabled" | ||
isOptional="true" | ||
> | ||
Determines whether the tab is disabled. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a Boolean? If so, "Determines" should be "Indicates" for consistency with wording elsewhere. I'd also add "to disable" after "whether" and delete "is disabled" to use active voice. Also, we label this property as optional, but the corresponding property on the tabs module is not labeled as optional. We need to update that property to label it as optional, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah the disabled boolean will default to false, so it should be optional. |
||
</sky-demo-page-property> | ||
</sky-demo-page-properties> | ||
|
||
|
||
<sky-demo-page-properties sectionHeading="Vertical tab group properties"> | ||
<sky-demo-page-property | ||
propertyName="groupHeading" | ||
> | ||
Specifies the text to use in the heading of the collapsible group of tabs. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd change "text to use in the heading of" to "header for." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. |
||
</sky-demo-page-property> | ||
<sky-demo-page-property | ||
propertyName="disabled" | ||
isOptional="true" | ||
> | ||
Boolean value that can be set to disabled expanding and collapsing a collapsible group. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd change "Boolean value that can be set" to "Indicates whether" and change "disabled" to "disable." I'd also change "expanding and collapsing" to "the ability to open and collapse." (I assume "expand" should be "open" for consistency since the next property is Change "a" before "collapsible" to "the." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. |
||
</sky-demo-page-property> | ||
<sky-demo-page-property | ||
propertyName="open" | ||
isOptional="true" | ||
> | ||
Boolean value indicating if the collapsible group is open. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd change "Boolean value indicating if" to "Indicates whether." Also, should this have something like "when the tabset loads" at the end? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. Although I'm not sure we need "when the tabset loads". The boolean value will update each time the group is expanded/collapsed. |
||
</sky-demo-page-property> | ||
</sky-demo-page-properties> | ||
|
||
<sky-demo-page-properties sectionHeading="Vertical tab events"> | ||
<sky-demo-page-property | ||
propertyName="activeChange" | ||
> | ||
Fires when the active tab changes. Emits the index of the active tab. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the second sentence end with something like "that is based on its position when it loads"? If accurate, would that be useful? (Not really sure if that's accurate, but assuming so based on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it is based on how it loads starting with 0.
|
||
</sky-demo-page-property> | ||
</sky-demo-page-properties> | ||
|
||
<sky-demo-page-example> | ||
<sky-vertical-tabs-demo></sky-vertical-tabs-demo> | ||
<sky-demo-page-code demoName="Vertical tabs"></sky-demo-page-code> | ||
</sky-demo-page-example> | ||
|
||
</sky-demo-page> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
<sky-vertical-tabset | ||
showTabsText="Tab list" | ||
(activeChange)="tabChanged($event)" | ||
> | ||
<sky-vertical-tabset-group | ||
*ngFor="let group of groups" | ||
[groupHeading]="group.heading" | ||
[open]="group.isOpen" | ||
[disabled]="group.isDisabled" | ||
> | ||
<sky-vertical-tab | ||
*ngFor="let tab of group.subTabs" | ||
[active]="tab.active" | ||
[tabHeading]="tab.tabHeading" | ||
[tabHeaderCount]="tab.tabHeaderCount" | ||
[disabled]="tab.disabled" | ||
> | ||
{{ tab.content }} | ||
</sky-vertical-tab> | ||
</sky-vertical-tabset-group> | ||
</sky-vertical-tabset> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { Component } from '@angular/core'; | ||
|
||
@Component({ | ||
selector: 'sky-vertical-tabs-demo', | ||
templateUrl: './vertical-tabs-demo.component.html' | ||
}) | ||
export class SkyVerticalTabsDemoComponent { | ||
|
||
public groups: any[]; | ||
|
||
constructor() { | ||
this.groups = [ | ||
{ | ||
heading: 'Group 1', | ||
isOpen: false, | ||
isDisabled: false, | ||
subTabs: [ | ||
{ tabHeading: 'Group 1 - Tab 1', content: 'Group 1 - Tab 1 Content'}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very minor, but I'd recommend replacing the hyphens here with long dashes. So "—" instead of "-" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok |
||
{ tabHeading: 'Group 1 - Tab 2', content: 'Group 1 - Tab 2 Content', tabHeaderCount: 7}] | ||
}, | ||
{ | ||
heading: 'Group 2', | ||
isOpen: true, | ||
isDisabled: false, | ||
subTabs: [ | ||
{ tabHeading: 'Group 2 - Tab 1', content: 'Group 2 - Tab 1 Content', active: true}, | ||
{ tabHeading: 'Group 2 - Tab 2 - Disabled', | ||
content: 'Group 2 - Tab 2 Content', | ||
disabled: true | ||
}] | ||
}, | ||
{ | ||
heading: 'Disabled', | ||
isOpen: false, | ||
isDisabled: true, | ||
subTabs: [] | ||
} | ||
]; | ||
} | ||
|
||
public tabChanged(newIndex: any) { | ||
console.log(`new active ${newIndex}`); | ||
} | ||
} |
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.
title
should bepageTitle
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.
Ok