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

Buttons default to normal case #226

Merged
merged 2 commits into from
Jul 13, 2023
Merged

Conversation

DannyDelott
Copy link
Contributor

@DannyDelott DannyDelott commented Jul 13, 2023

Buttons with all-caps labels are hard to read, especially when including abbreviations, like ADD LP.

@vercel
Copy link

vercel bot commented Jul 13, 2023

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

Name Status Preview Comments Updated (UTC)
hyperdrive-fixed-borrow ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 13, 2023 10:01pm
hyperdrive-monorepo-hyperdrive-trading ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 13, 2023 10:01pm

@@ -86,11 +85,8 @@ export function CloseLongForm({
)}

{account ? (
<Button
disabled={!closeLong || isPendingWalletAction}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this disabled logic? Now isPendingWalletAction is unused above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good catch, yes we do!

@@ -29,7 +29,7 @@ export function OpenShortModalButton({
{/* Using a div styled as a button here just as a visual cue. Don't
use a real button here since the Well is interactive already, and
doing so would create invalid dom nesting of buttons. */}
<div className="daisy-btn-accent daisy-btn-sm daisy-btn mt-2 justify-between gap-0 normal-case">
<div className="daisy-btn-accent daisy-btn-sm daisy-btn mt-2 justify-between gap-0">
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to be using this class string in a lot of places, it might make sense to place it into a tailwind component.
https://tailwindcss.com/docs/reusing-styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I could see this whole "Well w/ Call-To-Action inside it" becoming it's own component that we re-use.

Something like this eventually:

<CallToActionWell 
  variant="primary" // "primary" | "secondary" | "accent"
  icon={...}
  header="Open Short"
  description="..." 
  action={<button>{...}</button>}
  onClick={...} 
/>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants