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

Add Tabs component #73

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Add Tabs component #73

merged 2 commits into from
Dec 3, 2024

Conversation

dgca
Copy link
Contributor

@dgca dgca commented Dec 3, 2024

@dgca dgca requested a review from cgjohn December 3, 2024 19:56
function List({ children }: ListProps) {
return (
<HStack justifyContent="center" borderBottomWidth={1} borderColor="divider">
{children}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to make things more accessible, we should have the children here be a ul and then we can wrap the trigger elements in an li.

The ul can then also have the role of tablist and each li can have a role of tab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Man, this would be so much easier with polymorphic components :(

function Content({ children, value }: ContentProps) {
const context = useTabsContext();

return context.activeTab === value ? <>{children}</> : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

the content of a tab content area should have a role of tabpanel


return (
<html.button
onClick={() => context.setActiveTab(value)}
Copy link
Contributor

Choose a reason for hiding this comment

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

if active we can add an aria-selected value here so it reads out as the active tab

Copy link
Contributor

@cgjohn cgjohn left a comment

Choose a reason for hiding this comment

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

only thoughts are around a11y

@dgca
Copy link
Contributor Author

dgca commented Dec 3, 2024

Talked on Slack and decided to fix the a11y stuff as a follow up after trying to make polymorphic

@dgca dgca merged commit eed682e into main Dec 3, 2024
2 checks passed
@dgca dgca deleted the balances-pt-2 branch December 3, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants