Vertical tabs #1004
Vertical tabs #1004
Conversation
src/app/components/tabs/index.html
Outdated
| @@ -14,6 +14,35 @@ | |||
|
|
|||
| </sky-demo-page-properties> | |||
|
|
|||
| <sky-demo-page-properties sectionHeading="Vertical tabset properties"> | |||
There was a problem hiding this comment.
These can probably be removed?
There was a problem hiding this comment.
Well, there is a difference between properties on the whole tabset and properties on the individual tabs. Is there a different term I should use?
There was a problem hiding this comment.
Why do we need to document the vertical tabs properties on the "standard" tabs page (since vertical tabs has its own page now)? Are these properties also used here? Sorry, I might be misunderstanding...
There was a problem hiding this comment.
Oh, that's totally right I didn't realize I left this on the tabs documentation page. I thought you were commenting on the Vertal tabset properties vs Vertabl tab properties. I'll remove the changes to the tabs docs page.
| public chevronClick() { | ||
| this.direction = this.direction === 'up' ? 'down' : 'up'; | ||
| this.directionChange.emit(this.direction); | ||
| if (!this.disabled) { |
There was a problem hiding this comment.
The chevron component uses a native button element, which respects the disabled attribute, so I don't believe there's a reason to add conditions for tab index, etc. since the browser will turn those features off by default.
Unless I'm missing something critical, you can simply:
chevron.component.ts
@Input()
public disabled = false;
chevron.component.html
<button
type="button"
class="sky-chevron"
[ngClass]="['sky-chevron-' + direction]"
(click)="$event.stopPropagation();chevronClick()"
[attr.aria-label]="(direction === 'down' ? 'chevron_expand' : 'chevron_collapse') | skyResources"
[disabled]="disabled">
<i class="sky-chevron-part sky-chevron-left"></i>
<i class="sky-chevron-part sky-chevron-right"></i>
</button>
There was a problem hiding this comment.
I add the tabindex because I don't want it to be tabbable even if it isn't disabled. In my case the tab group is tabbable but I don't want both the tab group and the button to be tabbable. I should add the [disabled] attribute to the button though.
There was a problem hiding this comment.
I guess I could go the other way and make the chevron tabbable and not the group div. I was just copying how it was done in skyux 1.
There was a problem hiding this comment.
I checked and that is how it is done in the Tile component. I'll copy that instead of skyux 1
There was a problem hiding this comment.
Done. Using style of tiles in skyux 2 by making the chevron tabbable.
There was a problem hiding this comment.
Awesome! Works great.
I removed the if (!this.disabled) { condition and it still worked as expected. Not sure if that's needed...
| [ngClass]="{ | ||
| 'sky-vertical-tab-active': active, | ||
| 'sky-no-outline': !outline, | ||
| 'sky-outline': outline, |
There was a problem hiding this comment.
Preferably, the component should not know about the HTML's styles (i.e., outline). If the tab was an anchor element, it could be styled according to its native :focus attribute, and you wouldn't have to worry about keeping track of the outline boolean, or if the tab has focus.
.sky-vertical-tab:focus { // outline styles here }
Let me know if I'm missing something here, in the implementation!
There was a problem hiding this comment.
Done. I removed the outline logic and changed it to a div.
| </span> | ||
| </div> | ||
| <div #tabContentWrapper> | ||
| <div [ngClass]="{'sky-vertical-tab-hidden': !active, 'sky-vertical-tab-visible': active }"> |
There was a problem hiding this comment.
Unsure why you need two classes for the visibility? Either it's visible by default, and hidden with a class, or vice versa?
There was a problem hiding this comment.
I was using that second class to grab the visible tabs in my unit tests. Should I use a different selector instead. Like id="verticalTab" without the hidden class?
There was a problem hiding this comment.
Done. I removed the second class and added an id I could use with a css selector.
…tab group instead.
|
|
||
| <div #tabContentWrapper> | ||
| <div | ||
| id="verticalTab" |
There was a problem hiding this comment.
I know you need to target this somehow in your unit tests, but I'd make the argument that it's not a good idea to use IDs in component templates because there could potentially be more than one on a given page.
Instead, you could add a default class to this div and reference that in your unit test:
<div
class="sky-vertical-tab-content-pane"
[ngClass]="{ 'sky-vertical-tab-hidden': !active }">
<ng-content></ng-content>
</div>
el.querySelectorAll('.sky-vertical-tab-content-pane:not(.sky-vertical-tab-hidden)')
| 'sky-vertical-tab-active': active, | ||
| 'sky-vertical-tab-disabled': disabled | ||
| }" | ||
| (click)="activateTabFromClick()" |
There was a problem hiding this comment.
You can probably do away with this method and simply call activateTab() again?
|
|
||
| .sky-vertical-tab { | ||
| cursor: pointer; | ||
| padding: $sky-padding-3_4 0 $sky-padding-3_4 $sky-padding; |
There was a problem hiding this comment.
Nice! I like the use of SCSS variables, here.
| > | ||
| <ng-content></ng-content> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Do you mind making the classnames a bit more specific?
vertical-tabset-group.component.html
.sky-tab-group-container --> .sky-vertical-tabset-group
.sky-tab-group-header --> .sky-vertical-tabset-group-header
.sky-tab-chevron --> .sky-vertical-tabset-group-header-chevron
.sky-tab-group-content --> .sky-vertical-tabset-group-content
| {{ showTabsText }} | ||
| </button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Same here, please:
vertical-tabset.component.html
.sky-vertical-tabset-container --> .sky-vertical-tabset
.sky-vertical-tabset-show-tabs --> .sky-vertical-tabset-show-tabs-btn
|
@Blackbaud-AdamHickey This is very clean; I especially liked your component classes. @Blackbaud-ToddRoberts: I'm assuming you've approved the overall styles for the vertical tabs (note the addition of the
|
|
Yup UI is good to go. The counter is good since it copies sectioned forms in UX1. |
| @@ -0,0 +1,78 @@ | |||
| <sky-demo-page title="Vertical tabs"> | |||
There was a problem hiding this comment.
title should be pageTitle
| propertyName="showTabsText" | ||
| isOptional="true" | ||
| > | ||
| Specifies the text to use for the show tabs button in mobile. |
There was a problem hiding this comment.
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.
Yeah that is better.
|
|
||
| <sky-demo-page-properties sectionHeading="Vertical tab properties"> | ||
| <sky-demo-page-property | ||
| propertyName="active" |
There was a problem hiding this comment.
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 showTabsText property is not. So should we include the brackets in the name? Or is there some other way that developers should know which properties need brackets and which don't?
There was a problem hiding this comment.
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"
input="stringLiteral"
That is angular behavior and I don't think we have to document that.
| propertyName="active" | ||
| isOptional="true" | ||
| > | ||
| Indicates whether or not the tab is active on load |
There was a problem hiding this comment.
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.
Yeah agree.
| <sky-demo-page-property | ||
| propertyName="tabHeading" | ||
| > | ||
| The text of the button that selects the tab. |
There was a problem hiding this comment.
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 groupHeading description, we refer to "the collapsible group of tabs," not "the collapsible group of buttons that select tabs." ... All of which is just a long way to ask, can we just say something like "The text to display on the tab." or "Specifies the tab header."? (I prefer the latter. ... Also, if we change this, I can go make the same edit to the corresponding property on the tabs module.)
There was a problem hiding this comment.
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 | ||
| propertyName="groupHeading" | ||
| > | ||
| Specifies the text to use in the heading of the collapsible group of tabs. |
There was a problem hiding this comment.
I'd change "text to use in the heading of" to "header for."
| 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.
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 open, not expand.)
Change "a" before "collapsible" to "the."
| propertyName="open" | ||
| isOptional="true" | ||
| > | ||
| Boolean value indicating if the collapsible group is open. |
There was a problem hiding this comment.
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.
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 | ||
| propertyName="activeChange" | ||
| > | ||
| Fires when the active tab changes. Emits the index of the active tab. |
There was a problem hiding this comment.
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 tabIndex property in the tabs module and the fact that there isn't a way here to set the index.)
There was a problem hiding this comment.
Yeah, it is based on how it loads starting with 0.
<group>
<tab1 index = 0>
<tab2 index = 1>
</group>
<group>
<tab3 index = 2>
</group>
| isOpen: false, | ||
| isDisabled: false, | ||
| subTabs: [ | ||
| { tabHeading: 'Group 1 - Tab 1', content: 'Group 1 - Tab 1 Content'}, |
There was a problem hiding this comment.
Very minor, but I'd recommend replacing the hyphens here with long dashes. So "—" instead of "-"
Adding vertical tabs to skyux 2 for issue #573
To reduce complexity I chose to create a separate vertical-tabs component instead of adding a vertical tab option on the existing tabs component. I can change this if necessary.
I am doing some DOM manipulation via a shared service to get the tab content to appear to the right. I'm open to a better way if possible.