From 46f23dcf88e9f26445b4f552f90db455f7cad50d Mon Sep 17 00:00:00 2001
From: Tim deBoer <git@tdeboer.ca>
Date: Thu, 7 Mar 2024 14:25:56 -0500
Subject: [PATCH] fix: consistent button height

The height of primary/secondary/link buttons is inconsistent, because some
have borders and some don't. Secondary buttons get smaller when they're
in-progress or disabled, because the border is removed.

This modifies the default y padding to account for borders, and change
the secondary button to always have a border.

I tried making all the buttons have border (to match the current bg where
applicable) but that was more change, changed the styling slightly, and I
couldn't find a good way to match border opacity for the link button.
The multiple ? is ugly, but far shorter than that alternative.

Tab height unchanged (31h).

Before:
- link/primary/disabled secondary: 29.5h
- secondary: 31.5h (29.5 when disabled)
- danger: 33.5h

After: all 29.5h

Fixes #6304.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
---
 packages/renderer/src/lib/ui/Button.spec.ts | 16 ++++++++++++++++
 packages/renderer/src/lib/ui/Button.svelte  |  7 +++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/packages/renderer/src/lib/ui/Button.spec.ts b/packages/renderer/src/lib/ui/Button.spec.ts
index 661b11b7cf7..8d27b43992e 100644
--- a/packages/renderer/src/lib/ui/Button.spec.ts
+++ b/packages/renderer/src/lib/ui/Button.spec.ts
@@ -30,6 +30,8 @@ test('Check primary button styling', async () => {
   const button = screen.getByRole('button');
   expect(button).toBeInTheDocument();
   expect(button).toHaveClass('bg-purple-600');
+  expect(button).toHaveClass('border-none');
+  expect(button).toHaveClass('py-[5px]');
   expect(button).toHaveClass('text-[13px]');
   expect(button).toHaveClass('text-white');
 });
@@ -41,6 +43,7 @@ test('Check disabled/in-progress primary button styling', async () => {
   const button = screen.getByRole('button');
   expect(button).toBeInTheDocument();
   expect(button).toHaveClass('bg-charcoal-50');
+  expect(button).toHaveClass('py-[5px]');
   expect(button).toHaveClass('text-[13px]');
 });
 
@@ -61,6 +64,8 @@ test('Check secondary button styling', async () => {
   const button = screen.getByRole('button');
   expect(button).toBeInTheDocument();
   expect(button).toHaveClass('border-gray-200');
+  expect(button).toHaveClass('border-[1px]');
+  expect(button).toHaveClass('py-[4px]');
   expect(button).toHaveClass('text-[13px]');
   expect(button).toHaveClass('text-white');
 });
@@ -72,6 +77,9 @@ test('Check danger button styling', async () => {
   const button = screen.getByRole('button');
   expect(button).toBeInTheDocument();
   expect(button).toHaveClass('border-red-600');
+  expect(button).toHaveClass('border-2');
+  expect(button).toHaveClass('px-4');
+  expect(button).toHaveClass('py-[3px]');
   expect(button).toHaveClass('bg-charcoal-700');
   expect(button).toHaveClass('text-[13px]');
   expect(button).toHaveClass('text-white');
@@ -83,6 +91,10 @@ test('Check disabled/in-progress secondary button styling', async () => {
   // check for a few elements of the styling
   const button = screen.getByRole('button');
   expect(button).toBeInTheDocument();
+  expect(button).toHaveClass('border-charcoal-50');
+  expect(button).toHaveClass('border-[1px]');
+  expect(button).toHaveClass('px-4');
+  expect(button).toHaveClass('py-[4px]');
   expect(button).toHaveClass('bg-charcoal-50');
   expect(button).toHaveClass('text-[13px]');
 });
@@ -94,6 +106,8 @@ test('Check link button styling', async () => {
   const button = screen.getByRole('button');
   expect(button).toBeInTheDocument();
   expect(button).toHaveClass('border-none');
+  expect(button).toHaveClass('px-4');
+  expect(button).toHaveClass('py-[5px]');
   expect(button).toHaveClass('hover:bg-white');
   expect(button).toHaveClass('hover:bg-opacity-10');
   expect(button).toHaveClass('text-[13px]');
@@ -106,6 +120,8 @@ test('Check disabled/in-progress link button styling', async () => {
   // check for a few elements of the styling
   const button = screen.getByRole('button');
   expect(button).toBeInTheDocument();
+  expect(button).toHaveClass('px-4');
+  expect(button).toHaveClass('py-[5px]');
   expect(button).toHaveClass('text-charcoal-50');
   expect(button).toHaveClass('text-[13px]');
 });
diff --git a/packages/renderer/src/lib/ui/Button.svelte b/packages/renderer/src/lib/ui/Button.svelte
index e98a4b5eea9..253f234f288 100644
--- a/packages/renderer/src/lib/ui/Button.svelte
+++ b/packages/renderer/src/lib/ui/Button.svelte
@@ -15,7 +15,8 @@ export let selected: boolean | undefined = undefined;
 $: if (selected !== undefined && type !== 'tab') {
   console.error('property selected can be used with type=tab only');
 }
-export let padding: string = type !== 'tab' ? 'px-4 py-[5px]' : 'px-4 pb-1';
+export let padding: string =
+  'px-4 ' + (type === 'tab' ? 'pb-1' : type === 'secondary' ? 'py-[4px]' : type === 'danger' ? 'py-[3px]' : 'py-[5px]');
 
 let iconType: string | undefined = undefined;
 
@@ -30,8 +31,10 @@ onMount(() => {
 let classes = '';
 $: {
   if (disabled || inProgress) {
-    if (type === 'primary' || type === 'secondary') {
+    if (type === 'primary') {
       classes = 'bg-charcoal-50';
+    } else if (type === 'secondary') {
+      classes = 'border-[1px] border-charcoal-50 bg-charcoal-50';
     } else if (type === 'danger') {
       classes = 'border-2 border-gray-700 bg-charcoal-800';
     } else {