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.

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

fix(ktabs): slotted anchor link #2480

Merged
merged 9 commits into from
Oct 25, 2024
Merged

fix(ktabs): slotted anchor link #2480

merged 9 commits into from
Oct 25, 2024

Conversation

portikM
Copy link
Member

@portikM portikM commented Oct 25, 2024

Summary

The issue:
When a link is slotted into anchor slot of a tab, when using keyboard nav, the user needs to press tab twice to navigate to next element (first time the wrapper element div.tab-link has focus, then the slotted link element).

Solution:
Add anchorTabindex prop that allows set tabindex for div.tab-link element. This allows to make only one element in tab focusable.

Screen.Recording.2024-10-25.at.11.24.24.AM.mov

@portikM portikM self-assigned this Oct 25, 2024
Copy link

netlify bot commented Oct 25, 2024

Deploy Preview for kongponents-sandbox ready!

Name Link
🔨 Latest commit 3415584
🔍 Latest deploy log https://app.netlify.com/sites/kongponents-sandbox/deploys/671be0cfec1b000008c0a230
😎 Deploy Preview https://deploy-preview-2480--kongponents-sandbox.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 25, 2024

Deploy Preview for kongponents ready!

Name Link
🔨 Latest commit 3415584
🔍 Latest deploy log https://app.netlify.com/sites/kongponents/deploys/671be0cf57d0bb0008758d45
😎 Deploy Preview https://deploy-preview-2480--kongponents.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@portikM portikM marked this pull request as ready for review October 25, 2024 15:46
Comment on lines +521 to +542
<template>
<KTableView
row-key="id"
:data="tableData"
:headers="headers"
/>
</template>

<script setup lang="ts">
import type { TableViewData } from '@kong/kongponents'

const tableData: TableViewData = [
{
id: 'a70642b3-20f2-4658-b459-fe1cbcf9e315',
...
},
{
id: '58c599a9-f453-41f3-9e64-0a7fc6caedad',
...
}
]
</script>
Copy link
Member Author

Choose a reason for hiding this comment

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

Just making an example easier to understand

@portikM portikM enabled auto-merge (squash) October 25, 2024 15:48
Comment on lines 79 to 81
type: String,
default: '0',
validator: (val: string): boolean => val === '0' || val === '-1',
Copy link
Member

Choose a reason for hiding this comment

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

question: tabindex is always a number (even though it's passed as a string), why is this typed as a string? I think it might be easier to validate the provided value if the prop itself was a number.

e.g. if the user passes my-wrong-value as a string, the validator will error; however, you'd still be passing the invalid value directly to the HTML. I think this should be prevented.

Also, I'm not sure the validator makes sense either given the description of the prop. The logic of the validator prevents me from passing a tabindex of any value other than 0 or -1

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed. Added a check to make sure the passed value is either 0 or -1 and made the type more strict

Copy link
Member

Choose a reason for hiding this comment

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

why can the value only be 0 or -1? if we have a prop to customize the tabindex, why can't we allow any value?

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 forgot it can be other values 😅
Fixed now, accepts value -1 <= x <= 32767 (mdn)

src/components/KTabs/KTabs.vue Show resolved Hide resolved
Co-authored-by: Adam DeHaven <2229946+adamdehaven@users.noreply.github.com>
Copy link
Member

@adamdehaven adamdehaven left a comment

Choose a reason for hiding this comment

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

lgtm

@portikM portikM merged commit 72ae295 into main Oct 25, 2024
11 checks passed
@portikM portikM deleted the fix/ktabs-slotted-anchor-link branch October 25, 2024 20:47
kongponents-bot pushed a commit that referenced this pull request Oct 25, 2024
## [9.13.1](v9.13.0...v9.13.1) (2024-10-25)

### Bug Fixes

* **ktabs:** slotted anchor link ([#2480](#2480)) ([72ae295](72ae295))
@kongponents-bot
Copy link
Collaborator

🎉 This PR is included in version 9.13.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants