AP-543 Refactor button implementation

[justinanastos]

There are a lot of subtlties involved in the colors and behavior of the
button. This allows us to pass a few configuration options in, including a
color prop, and all these color properties will be handled automatically.

[trevorblades]

This object and the one above it in the array could be merged together:

[
  {'&[disabled]': ...},
  {backgroundColor: ...},
  // ...other stuff
]
// can be...
[
  {
    '&[disabled]': ...,
    backgroundColor: ....,
    // ...other keys
  },
  // ...other objects
]

[justinanastos]

@cheapsteak explicitly separated them out, but I don't remember why. I think I'll leave it for now. This whole thing could use a clean-up, but it should be ready to ship now.

Ship when it's useful, not perfect!

[cheapsteak]

This is the file from when I converted the original single object into arrays of objects
https://github.com/apollographql/space-kit/blob/f2ed4b77f16d6412a9ec0dc0ce5d01abdea45dd2/src/Button.tsx

At the time there was a conditional variant === "simple" && { backgroundColor: "transparent" }, separating the 1st block and the 3rd block.

space-kit/src/Button.tsx

Lines 105 to 108 in f2ed4b7

	},
	variant === "simple" && { backgroundColor: "transparent" },
	{

It looks like that's been removed so these two should be good to merge into one object (non blocking of course)

[justinanastos]

Thanks @cheapsteak ! Always good to get context from the source. Since this hasn't been approved by anyone I'll make the change.

non blocking of course

Very much appreciate when people write this.

[trevorblades]

Looks good @justinanastos! Just a few small comments above 👍

[mayakoneval]

I still am unable to override the text color of these buttons unless I wrap the text in its own text color, why is the default black for a red button? To fix, could we add a classNames prop?

[justinanastos]

@mayakoneval

I still am unable to override the text color of these buttons unless I wrap the text in its own text color, why is the default black for a red button? To fix, could we add a classNames prop?

You can't override with tailwind yet because of emotion. There's a way to get past that but I haven't figured it out out. We don't need to add a className prop because all props that aren't destructured are passed through to to the <button>, including className. In the meantime cheat: use style={{ color: colors.white }}.

[adamatorres]

@justinanastos -- Here's what I'm seeing needs to change:

• basic simple button and simple button with icon and both light and dark versions, the focus state text color should be blue.
• The examples for the default and small buttons, the alignment of the text is off vertical center, but like 1 or 2 pixels.

[cheapsteak]

Gonna do some head-down coding (don't block on me)

Will leave this review to the other capable eyes here

[justinanastos]

@adamatorres great eyes. the icons and text alignment were also off in the simple buttons. Should all be correct now!