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

feat: add Tabs component #1864

Merged
merged 20 commits into from
Jan 24, 2025
Merged

feat: add Tabs component #1864

merged 20 commits into from
Jan 24, 2025

Conversation

abcrane123
Copy link
Contributor

@abcrane123 abcrane123 commented Jan 23, 2025

What changed? Why?

  • add simple tabs component to be used in Earn
  • putting in internal in case we have another use for tabs in the future
          <Tabs defaultValue="deposit">
            <TabsList>
              <Tab value="deposit">Deposit</Tab>
              <Tab value="withdraw">Withdraw</Tab>
            </TabsList>
            <TabContent value="deposit">Deposit</TabContent>
            <TabContent value="withdraw">Withdraw</TabContent>
          </Tabs>

Notes to reviewers

How has it been tested?

Copy link

vercel bot commented Jan 23, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
onchainkit-coverage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2025 11:36pm
onchainkit-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2025 11:36pm
onchainkit-routes ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2025 11:36pm

<div
className={cn(
border.lineDefault,
'flex overflow-hidden !border-r-0 !border-l-0 !border-t-0',
Copy link
Contributor

Choose a reason for hiding this comment

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

why do these need to be important?

Copy link
Contributor Author

@abcrane123 abcrane123 Jan 23, 2025

Choose a reason for hiding this comment

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

when you add the border.lineDefault class it seems to take priority even if you pass border-r-0 after and we need to use border.lineDefault to ensure the border is consistent with rest of theme

Copy link
Contributor Author

@abcrane123 abcrane123 Jan 23, 2025

Choose a reason for hiding this comment

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

but now that i've refactored i removed and will rely on card to handle border

className,
)}
onClick={handleClick}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

better for accessibility if this was actually a button, divs aren't focusable elements

@@ -0,0 +1,70 @@
import { fireEvent, render, screen } from '@testing-library/react';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nit but could you also change the directory name from components/Tabs to components/tabs? I know we have a couple here that don't follow that pattern, but it's bad practice that we should try and avoid

e.g.:
https://x.com/kentcdodds/status/1874830406140448943

@abcrane123 abcrane123 merged commit 491c04d into main Jan 24, 2025
16 checks passed
@abcrane123 abcrane123 deleted the alissa.crane/tabs branch January 24, 2025 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants