-
Notifications
You must be signed in to change notification settings - Fork 0
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 navigation #535
base: main
Are you sure you want to change the base?
Add Tabs navigation #535
Conversation
🔍 Visual review for your branch is published 🔍Here are the links to: |
lib/ui/tab-navigation.tsx
Outdated
// base | ||
"flex items-center justify-center whitespace-nowrap rounded-xl px-3 py-1.5 font-medium transition-all", | ||
// background | ||
type === "primary" |
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.
let's maybe gather all the classes for primary and secondary to an object and inject it all together?
const classes = {
primary: "bg-f1-background-transparent group-hover:bg-f1-background-secondary group-hover:text-f1-foreground",
secondary: "bg-f1-background/60 group-hover:border-f1-border"
}
Then we can get it by the type variable
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.
Done with cva
, not sure if it looks cleaner now though that secondary is a boolean 😅
component: Tabs, | ||
tags: ["autodocs"], | ||
argTypes: { | ||
type: { |
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.
looks like this control does not work, I don't see it in the storybook of the PR
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.
I changed this now that I switched secondary to a boolean. Can you check again?
} | ||
|
||
const tabNavigationVariants = cva( | ||
"flex items-center justify-start gap-1 overflow-x-auto whitespace-nowrap border-b border-b-f1-border-secondary px-6 py-3 [scrollbar-width:none] [&::-webkit-scrollbar]:hidden", |
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.
@nlopin not sure what's going on here, if I set the border this way, it is not displayed (that's why I was doing this weird thing with border-transparent etc earlier) 😅
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.
are you sure border-b
does something?
🚪 Why?
Tabs are part of the navigation components for Factorial One. This is one of the components we need for this first stage.
🔑 What?
tab-navigation
to ui folder (base component for this one).Tabs
to experimental.Screen.Recording.2024-09-18.at.15.22.18.mov
Tabs motion example
Tabs has a
type
prop that changes the style of the tab bar, as there's an option to have two tabs bars in some sections.Tabs with secondary type
🏡 Context
🎨 Tokens
border-secondary
, used in non-interactive elements and separators.Note
PD: There is already a Tabs component in our codebase, that is based on the Radix Tab component, which is meant for tab panels. However, our Tabs function more like a Navigation Menu, and that's what I based this component on. This way, it adheres to the navigation role requirements.
We can remove the Tabs one, and adapt the Employee Profile to use this one.