Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(ktabs): allow tab anchors to be links [KHCP-13866] #2532

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+140 −79
Open

fix(ktabs): allow tab anchors to be links [KHCP-13866] #2532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d9f6fb8
fix(kcodeblock): ts error [KHCP-11924]
portikM Nov 29, 2024
5dcf284
fix(ktabs): allow tab anchores to be links [KHCP-13866]
portikM Nov 29, 2024
61943c2
fix: address pr feedback
portikM Dec 2, 2024
8452892
fix(ktabs): revert role navigation changes [KHCP-13866]
portikM Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 59 additions & 37 deletions docs/components/tabs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,18 @@ Required prop, which is an array of tab objects with the following interface:

```ts
interface Tab {
hash: string // has to be unique, corresponds to the panel slot name
hash: string
title: string
disabled?: boolean
disabled?: boolean,
to?: string | object
}
```

* `hash` - has to be unique, corresponds to the panel slot name
* `title` - title to be displayed in the tab
* `disabled` - whether or not tab is disabled
* `to` - if present, tab will be rendered as either a `router-link` or an `a`

<KTabs :tabs="tabsWithDisabled">
<template #tab1>
<p>Tab 1 content</p>
Expand Down Expand Up @@ -176,41 +182,6 @@ const tabChange = (hash: string): void => {
</script>
```

### anchorTabindex
Copy link
Member

Choose a reason for hiding this comment

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

This prop is being removed? This would be a breaking change, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically yes but this prop was added a bit more than a month ago and there is no usage of it in our repos (except for 2 occurrences that I introduced in MFEs but I will fix those up).

Copy link
Member

Choose a reason for hiding this comment

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

It's still a breaking change. It should be deprecated instead so we don't have to do a breaking release.

Copy link
Member Author

@portikM portikM Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, let me bring it back and deprecate it instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


This prop allows setting a custom `tabindex` for the tab anchor element. It’s useful when passing a custom interactive element, like a link, through the [`anchor` slot](#anchor-panel), ensuring that only the slotted element is focusable by resetting the default anchor `tabindex`. Default value is `0`.

#### Dynamic RouterView

Here's an example (code only) of utilizing a dynamic `router-view` component within the host app:

```html
<KTabs
hide-panels
:tabs="tabs"
>
<template
v-for="tab in tabs"
:key="`${tab.hash}-anchor`"
#[`${tab.hash}-anchor`]
>
<router-link
:to="{
name: tab.hash.split('?').shift(),
hash: `#${tab.hash.split('?').pop()}`,
}"
>
{{ tab.title }}
</router-link>
</template>
</KTabs>

<router-view v-slot="{ route }">
<h3>Router View content</h3>
<p>{{ route.path }}{{ route.hash }}</p>
</router-view>
```

## Slots

### anchor & panel
Expand Down Expand Up @@ -297,6 +268,43 @@ const tabs = ref<Tab[]>([
</script>
```

## Usage

### Tab links

Passing `to` property for each tab object allows to render tabs as links. If a string is passed, it will be used in `href` attribute in the rendered `a` element. If an object is passed, the tab will be rendered as a `router-link`.

<KTabs :tabs="linkTabs" hide-panels v-model="linkTabValue" />

{{ linkTabValue }}

```vue
<template>
<KTabs :tabs="linkTabs" hide-panels />
<router-view v-slot="{ route }">
{{ route.hash }}
</router-view>
</template>
<script setup lang="ts">
import { Tab } from '@kong/kongponents'
const linkTabs = ref<Tab[]>([
{
hash: '#tab1',
title: 'Tab 1',
to: '#tab-link-1'
},
{
hash: '#tab2',
title: 'Tab 2',
to: '#tab-link-2'
},
])
</script>
```

<script setup lang="ts">
import { ref } from 'vue'
import { KongIcon, InboxNotificationIcon, BookIcon } from '@kong/icons'
Expand All @@ -322,6 +330,20 @@ const slottedTabs = ref<Tab[]>([
{ hash: '#disabled', title: 'Disabled', disabled: true }
])

const linkTabValue = ref<string>('#tab-link-1')
const linkTabs = ref<Tab[]>([
{
hash: '#tab-link-1',
title: 'Tab 1',
to: '#tab-link-1',
},
{
hash: '#tab-link-2',
title: 'Tab 2',
to: '#tab-link-2',
},
])

const panelsActiveHash = ref('#gateway')

const panelsChange = (hash: string) => {
Expand Down
20 changes: 4 additions & 16 deletions sandbox/pages/SandboxTabs.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,25 +111,11 @@
/>
<SandboxSectionComponent title="Dynamic router view without panels">
<KTabs
:anchor-tabindex="-1"
hide-panels
:tabs="dynamicRouterViewItems"
@change="(hash: string) => $router.replace({ hash })"
>
<template #one-anchor>
<router-link :to="{ hash: '#one' }">
One
</router-link>
</template>
<template #two-anchor>
<router-link :to="{ hash: '#two' }">
Two
</router-link>
</template>
</KTabs>
<router-view
v-slot="{route}"
>
/>
<router-view v-slot="{ route }">
<p>{{ route.path }}{{ route.hash }}</p>
</router-view>
</SandboxSectionComponent>
Expand Down Expand Up @@ -174,10 +160,12 @@ const dynamicRouterViewItems = [
{
title: 'One',
hash: '#one',
to: { hash: '#one' },
},
{
title: 'Two',
hash: '#two',
to: { hash: '#two' },
},
]
</script>
2 changes: 1 addition & 1 deletion src/components/KCodeBlock/KCodeBlock.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ const matchingLineNumbers = ref<number[]>([])
const currentLineIndex = ref<null | number>(null)
const totalLines = computed((): number[] => Array.from({ length: props.code?.split('\n').length }, (_, index) => index + 1))
const maxLineNumberWidth = computed((): string => totalLines.value[totalLines.value.length - 1]?.toString().length + 'ch')
const maxLineNumberWidth = computed((): string => totalLines.value[totalLines.value?.length - 1]?.toString().length + 'ch')
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 Drive-by comment: Is this a bug fix, and if so should it go in a different PR? If not should it be here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a bug fix. Since it's such a small change I think it's fine if it goes in in the same PR

const linePrefix = computed((): string => props.id.toLowerCase().replace(/\s+/g, '-'))
const isProcessing = computed((): boolean => props.processing || isProcessingInternally.value)
const isShowingFilteredCode = computed((): boolean => isFilterMode.value && filteredCode.value !== '')
Expand Down
36 changes: 34 additions & 2 deletions src/components/KTabs/KTabs.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('KTabs', () => {
})
})

it('hides the panel content when `hidePanels` is true', () => {
it('hides the panel content when hidePanels is true', () => {
const picturesSlot = 'I love pictures'
const moviesSlot = 'I love pictures'
const booksSlot = 'I love pictures'
Expand Down Expand Up @@ -75,7 +75,7 @@ describe('KTabs', () => {

// handles disabled item correctly

it('disables the tab item when `disabled` is true', () => {
it('disables the tab item when disabled is true', () => {
const tabs = [
{ hash: '#pictures', title: 'Pictures' },
{ hash: '#movies', title: 'Movies', disabled: true },
Expand All @@ -94,6 +94,38 @@ describe('KTabs', () => {
})
})

it('renders the tab as a link if tab.to is present', () => {
const tabs = [
{ hash: '#pictures', title: 'Pictures' },
{ hash: '#movies', title: 'Movies', to: '/movies' },
{ hash: '#books', title: 'Books' },
]

cy.mount(KTabs, {
props: {
tabs,
},
})

cy.get('.tab-item .tab-link').eq(1).should('have.attr', 'href', '/movies')
})

it('renders the tab as a link with no href attribute if tab.to is present and tab.disabled is true', () => {
const tabs = [
{ hash: '#pictures', title: 'Pictures' },
{ hash: '#movies', title: 'Movies', to: '/movies', disabled: true },
{ hash: '#books', title: 'Books' },
]

cy.mount(KTabs, {
props: {
tabs,
},
})

cy.get('.tab-item .tab-link').eq(1).should('not.have.attr', 'href')
})

describe('slots', () => {
it('provides the #hash slot content', () => {
const picturesSlot = 'I love pictures'
Expand Down
62 changes: 39 additions & 23 deletions src/components/KTabs/KTabs.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,24 @@
class="tab-item"
:class="{ active: activeTab === tab.hash }"
>
<div
<component
Copy link
Contributor

@johncowen johncowen Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if this is something you'll want to address in this PR, but if all of the "tabs" are using anchors, it means its essentially a role="navigation" (or just a nav). It just happens to look like what I suppose could generally be termed as a "tab look and feel". (In regards to line 5 here which I can't make a comment specifically on that line because GH)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, added a dynamic role for ul element depending on whether all tabs are links or not

Copy link
Contributor

@johncowen johncowen Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you are using the role="navigation" you'll probably want to let the user decide what the aria-label is.

Question: Would you consider a very simple KNavigation component as an alternative to the current approach that was just a <nav><ul><li> with some similar styling for the tabs (i.e. very minimal JS)?

I would guess a theoretical KNavigation would be an very simple component to write and way less likely for bugs to arise as you'd need about 5% of the JS you have in here. You would also then avoid complicating KTabs. I would guess that would also stop @adamdehaven sweating about possible breaking changes 😅 so a positive all round!

Copy link
Member

Choose a reason for hiding this comment

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

I think this is getting out of scope for the PR.

TBH the existing aria-label should be removed as it's generic and doesn't actually describe the content. "Tabs" doesn't label the type of content being displayed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but would be really good to make sure all of the native anchor functionality and accessibility is also maintained if you are sticking to this component. I believe we wrote something about us being accessibility focussed somewhere? 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that KNavigation is out of scope for this PR.
As for aria-label, it is an attribute required for elements with role="tablist" and role="navigation". Removing it will bite us back in the upcoming a11y audit for sure. Instead I suggest that we bind aria-label (when provided) to this element instead of topmost wrapper div and add a paragraph in docs mentioning that it would be a good idea to provide a custom value, or otherwise the default ("Tabs") will be used?

Copy link
Member

Choose a reason for hiding this comment

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

Again, I still think out of scope for this PR.

Can create a ticket to discuss for a future update

Copy link
Contributor

@johncowen johncowen Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok but if we are using role=navigation here make sure you remember to also remove all the other aria attributes and that aren't needed on role=navigation. Would also be good to remove or at least make sure that the custom events added to the lis here don't break any existing native functionality provided by using anchors. (edit: oh sorry the li's I mention here would now be a's if I understand correctly)

Whilst discussing this (and this might also be out of scope so let me know if so), I think you might want to remove some of these for when "hide-panels" is true also, as in that case its not really a role="tablist" either.

Let me know the link for that ticket @adamdehaven and I'll catch up with that tomorrow 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is getting out of scope of this PR. I will back out the change related to role="navigation" and create a separate ticket to address these concerns in a separate PR.

:is="tabComponent(tab).tag"
:aria-controls="hidePanels ? undefined : `panel-${tab.hash}`"
:aria-selected="hidePanels ? undefined : (activeTab === tab.hash ? 'true' : 'false')"
class="tab-link"
:class="{ 'has-panels': !hidePanels, disabled: tab.disabled }"
:class="{ disabled: tab.disabled }"
role="tab"
:tabindex="getAnchorTabindex(tab)"
v-bind="tabComponent(tab).attributes"
@click="!tab.disabled ? handleTabChange(tab.hash) : undefined"
@click.prevent="!tab.disabled ? handleTabChange(tab.hash) : undefined"
@keydown.enter.prevent="!tab.disabled ? handleTabChange(tab.hash) : undefined"
@keydown.space.prevent="!tab.disabled ? handleTabChange(tab.hash) : undefined"
>
<slot :name="`${getTabSlotName(tab.hash)}-anchor`">
<span>{{ tab.title }}</span>
adamdehaven marked this conversation as resolved.
Show resolved Hide resolved
</slot>
</div>
</component>
</li>
</ul>

Expand All @@ -49,7 +52,7 @@

<script lang="ts" setup>
import type { PropType } from 'vue'
import { ref, watch } from 'vue'
import { computed, ref, watch } from 'vue'

Check warning on line 55 in src/components/KTabs/KTabs.vue

View workflow job for this annotation

GitHub Actions / Run Component Tests 

'computed' is defined but never used
import type { Tab } from '@/types'

const props = defineProps({
Expand All @@ -75,6 +78,9 @@
type: Boolean,
default: false,
},
/**
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

did you mark as deprecated in the docs as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this prop from docs (our standard practice for deprecated props is to remove them from Kongponents docs).

Copy link
Member

Choose a reason for hiding this comment

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

I removed this prop from docs (our standard practice for deprecated props is to remove them from Kongponents docs).

Standard is actually to mark as deprecated

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Well maybe we can't use a word "standard" here since we have different patterns:

  • KModalFullscreen is deprecated and we don't have the component docs page
  • KTable is deprecated but because it's such a crucial component internally we still have the page up
  • label prop in KDropdown is deprecated and is not present in docs
  • same applies to isResizable and hasError props in KTextArea
  • in fact, same applies to every deprecated prop in any component

So how do we want to go about this one? In my opinion, we don't want to promote usage of it so we should remove it from docs.

*/
anchorTabindex: {
type: Number,
default: 0,
Expand Down Expand Up @@ -105,6 +111,18 @@
return typeof props.anchorTabindex === 'number' && props.anchorTabindex >= -1 && props.anchorTabindex <= 32767 ? String(props.anchorTabindex) : '0'
}

const tabComponent = (tab: Tab) => {
if (tab.to) {
if (typeof tab.to === 'string') {
return { tag: 'a', attributes: { href: tab.disabled ? undefined : tab.to } }
} else if (typeof tab.to === 'object') {
return { tag: 'router-link', attributes: { to: tab.disabled ? undefined : tab.to } }
}
}

return { tag: 'div', attributes: {} }
}

watch(() => props.modelValue, (newTabHash) => {
activeTab.value = newTabHash
emit('change', newTabHash)
Expand All @@ -125,11 +143,7 @@
ul {
border-bottom: var(--kui-border-width-10, $kui-border-width-10) solid var(--kui-color-border, $kui-color-border);
display: flex;
font-family: var(--kui-font-family-text, $kui-font-family-text);
font-size: var(--kui-font-size-30, $kui-font-size-30);
font-weight: var(--kui-font-weight-semibold, $kui-font-weight-semibold);
gap: var(--kui-space-40, $kui-space-40);
line-height: var(--kui-line-height-40, $kui-line-height-40);
list-style: none;
margin-bottom: var(--kui-space-70, $kui-space-70);
margin-top: var(--kui-space-0, $kui-space-0);
Expand All @@ -148,30 +162,23 @@
white-space: nowrap;

.tab-link {
@include defaultButtonReset;
align-items: center;

border-radius: var(--kui-border-radius-30, $kui-border-radius-30);
color: var(--kui-color-text-neutral, $kui-color-text-neutral);
cursor: pointer;
display: inline-flex;
font-family: var(--kui-font-family-text, $kui-font-family-text);
font-size: var(--kui-font-size-30, $kui-font-size-30);
font-weight: var(--kui-font-weight-semibold, $kui-font-weight-semibold);
gap: var(--kui-space-40, $kui-space-40);
line-height: var(--kui-line-height-40, $kui-line-height-40);
padding: var(--kui-space-30, $kui-space-30) var(--kui-space-50, $kui-space-50);
text-decoration: none;
transition: color $kongponentsTransitionDurTimingFunc, background-color $kongponentsTransitionDurTimingFunc, box-shadow $kongponentsTransitionDurTimingFunc;
user-select: none;

// Applies the padding to the tab’s content when not showing panels which is typically used for placing links inside KTabs for navigational tabs. Otherwise, clicking the tab outside of the link’s box will mark it as active but won’t actually navigate.
&.has-panels,
&:not(.has-panels) :deep(> *) {
padding: var(--kui-space-30, $kui-space-30) var(--kui-space-50, $kui-space-50);
}

a, :deep(a) {
color: var(--kui-color-text-neutral, $kui-color-text-neutral);
text-decoration: none;

&:focus-visible {
@include kTabsFocus;
}
}

&:hover:not(.disabled) {
background-color: var(--kui-color-background-neutral-weaker, $kui-color-background-neutral-weaker);
}
Expand All @@ -184,6 +191,15 @@
color: var(--kui-color-text-disabled, $kui-color-text-disabled);
cursor: not-allowed;
}

:slotted(a) {
color: var(--kui-color-text-neutral, $kui-color-text-neutral);
text-decoration: none;

&:focus-visible {
@include kTabsFocus;
}
}
}

&.active {
Expand Down
3 changes: 3 additions & 0 deletions src/types/tabs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
export interface Tab {
/** Has to be unique, corresponds to the panel slot name */
hash: string
title: string
disabled?: boolean
/** If present, tab will be rendered as either a router-link or an anchor */
to?: string | object
}
Loading