-
Notifications
You must be signed in to change notification settings - Fork 185
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 UI tweaks #1315
Feat UI tweaks #1315
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I think we should consider a more aggressive revision for some of these things at some point... but just want to get these working changes in |
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.
left some comments a couple things - thanks for cleaning this up!
import { Button } from '@/components/ui/button'; | ||
|
||
export interface BannerProps { | ||
message: React.ReactNode; |
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.
qq: can we make this a string?
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 think we may want to include an icon or additional links here (i.e. link to migration docs)
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.
cool, sounds good
showBanner?: boolean; | ||
} | ||
|
||
export const Banner: React.FC<BannerProps> = ({ |
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.
stylistic nit: I've been defining components like this:
export const Banner = ({foo, bar}: BannerProps) => {}
I don't care which we end up going for, but I think we should try to be consistent. WDYT? I kinda like the way I've been doing it because it's a little more terse, but I'm fine either way
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.
personally prefer React.FC since it adds some additional type safety if needed later
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.
ah nice, vs. just JSX.Element
? sounds good to me
Description
UI Tweaks for upgrade routes