-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update tab indicator implementation #578
Changes from 3 commits
5b93c60
f97511c
26290df
7d0a91e
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 |
---|---|---|
@@ -1,6 +1,6 @@ | ||
<template> | ||
<div class="ods-tabs" :id="id"> | ||
<div class="ods-tabs--tablist" ref="tablist" role="tablist" aria-label="" :style="indicatorStyle" @keydown.left.right.end.home.prevent="handleTabFocus"> | ||
<div class="ods-tabs--tablist" ref="tablist" role="tablist" aria-label="" @keydown.left.right.end.home.prevent="handleTabFocus"> | ||
<button | ||
class="ods-tabs--tab" | ||
role="tab" | ||
|
@@ -55,44 +55,33 @@ export default { | |
}, | ||
data() { | ||
return { | ||
indicator: { width: 0, x: 0 }, | ||
focusIndex: 0, | ||
focusCount: 0 | ||
} | ||
}, | ||
computed: { | ||
indicatorStyle() { | ||
return ` | ||
--ods-tabs-indicator-width: ${this.indicator.width}px; | ||
--ods-tabs-indicator-pos-x: ${this.indicator.x}px; | ||
` | ||
}, | ||
}, | ||
methods: { | ||
tabSelect (event) { | ||
const tab = event.target | ||
|
||
this.active = tab.id | ||
this.focusIndex = [...tab.parentElement.children].indexOf(tab) | ||
this.indicator = { | ||
width: tab.offsetWidth, | ||
x: tab.offsetLeft | ||
} | ||
}, | ||
handleTabFocus ({ key }) { | ||
if (key === 'ArrowLeft') { | ||
this.tabPrev() | ||
} | ||
else if (key === 'ArrowRight') { | ||
this.tabNext() | ||
} | ||
else if (key === 'End') { | ||
this.tabLast() | ||
switch (key) { | ||
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. Please add a 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. There is no reason for us to pick up any other keys. I'll admit this felt a little weird to me too, but since it was obvious we only needed to answer for those specific cases/keys I went with it. Would you prefer if I went back to a conditional? |
||
case 'ArrowLeft': | ||
this.tabPrev() | ||
break; | ||
case 'ArrowRight': | ||
this.tabNext() | ||
break; | ||
case 'End': | ||
this.tabLast() | ||
break; | ||
case 'Home': | ||
this.tabFirst() | ||
break; | ||
} | ||
else if (key === 'Home') { | ||
this.tabFirst() | ||
} | ||
|
||
|
||
this.focusItem() | ||
}, | ||
tabPrev () { | ||
|
@@ -131,11 +120,6 @@ export default { | |
|
||
this.focusCount = this.tablist.length - 1 // use zero index | ||
this.focusIndex = 1 | ||
|
||
this.indicator = { | ||
width: activeTab.offsetWidth, | ||
x: activeTab.offsetLeft | ||
} | ||
}, | ||
} | ||
</script> |
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.
As an FYI - this isn't a required aria attribute and isn't compatible with web components. Material UI and Polaris do not use these labels either.
In the future it would be nice to outline attributes as either MUST or SHOULD be implemented, since it'll depend on the technology used by consumers.