-
Notifications
You must be signed in to change notification settings - Fork 49
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
Major refactor #272
Major refactor #272
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
The component extraction part looks good to me. I don't really understand the query client stuff though, and I don't want to approve this without understanding that as I think there's some potential for weird caching behavior.
isActionDisabled: boolean | undefined | ||
actionLabel: string | ||
} | ||
export function BasePanel({ |
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.
nitpick: imo this isn't a great abstraction because it's pretty inflexible. It's hard to customize the panels using this component to be slightly different from each other.
I wouldn't worry about changing that in this PR though. if someone wants more customizability later, they can copy/paste this code into the other panel component.
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.
(a better abstraction would be more composable. An example here would be to make a component for the token input with the half/max buttons and the input text. If you had a couple of those little components, it would be easy to specify the LockPanel etc using a small number of lines of code, and it would also be more flexible in case you wanted to vary the appearances of those panels)
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 am going to address this in future. Prioritising other things over this.
No description provided.