-
Notifications
You must be signed in to change notification settings - Fork 181
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 Polo page #691
fix Polo page #691
Conversation
|
||
export function Tabs({ tabs, activeTab, onChange, className }: TabsProps) { | ||
return ( | ||
<div className={clsx("flex border-b border-[#252525] mb-6", className)}> |
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.
dont use direct colors in components, you should put or use existing one from @theme inside index.css
|
||
export function Tabs({ tabs, activeTab, onChange, className }: TabsProps) { | ||
return ( | ||
<div className={clsx("flex border-b border-[#252525] mb-6", className)}> |
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.
cn util should be used (which is already created) instead of clsx
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.
UI doesn't match figma.
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.
ui doesn't match figma
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.
color should not be put directly into component, here and in other places (e,g, text-[#848484])
</div> | ||
</div> | ||
); | ||
const [activeTab, setActiveTab] = useState<"all" | "stable" | "volatile">("all"); |
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.
use enum for this values
{ label: "Volatile", value: "volatile" }, | ||
]} | ||
activeTab={activeTab} | ||
onChange={(val) => setActiveTab(val as typeof activeTab)} |
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.
please don't use as typeof, use created enum for values
|
||
|
||
<div className="lg:hidden"> | ||
<Tabs |
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.
seems like we don't need to use tabs twice, we can just use it once and below, conditionally render either pool row or pool card, depending on the device (mobile/desktop)
> | ||
{tab.label} | ||
{activeTab === tab.value && ( | ||
<span className="absolute bottom-0 left-0 w-full h-[2px] bg-[#ffb80d]" /> |
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 can't find such color in figma, why did you decide to use it? (#ffb80d)
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.
remove, use yarn
{pool.riskLevel} | ||
</td> | ||
<td className="px-4 py-3 whitespace-nowrap"> | ||
<button className="bg-[#1b1b1b] hover:bg-[#252525] transition-colors px-4 py-2 rounded text-sm"> |
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.
we have own Button component, please use it
</Button> | ||
</div> | ||
); | ||
<button className="w-full py-3 bg-[#1b1b1b] hover:bg-[#252525] rounded text-sm transition-colors"> |
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.
we have own Button component, please use it
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.
please fix styles to match figma
I want to have pair of screenshots:
on the left - Figma version
on the right - your version
it should be 100% identical in terms of styling
@Uyoxy Any updates? |
No description provided.