Add button link as styles function, introduce tailwind-merge

[DTCurrie]

Proposing an idea here. We want to move the button link from app to prime, but after some recent conversations, I wonder if we instead just want to export a function to generate the styles for a button link. It will allow us to use the tools already built to support a native <a /> element, including things like autocompleting for rel and target, which our button link implementation limits. It also creates pretty easy-to-test little units.

Another thing I introduced was tailwind-merge, which works a lot like classnames but also merges tailwind classes down to avoid conflicting styles. With the way we use extraClasses this allows an easy way to actually override styles without having to use important tags.

[DTCurrie]

Just some import sorting.

[DTCurrie]

The linter actually hates this, will fix later.

[DTCurrie]

I will create a better example if we decide to go this way.

[ethanlookpotts]

Haven't reviewed the rest of this PR yet but I wouldn't expect a 22k line diff for the pnpm-lock. pnpm dedupe?

[DTCurrie]

Ran it, still getting those changes. Seems like it is bumping the lockfile version from 6.0 to 9.0 and doing any allowed minor/patch bumps on all dependencies. I freshly cloned the repo and installed so may be related to that.

[mcous]

I am very much in favor of this change, but I think for now, we should avoid tailwind-merge. From their docs:

Reasons not to use it

Generally speaking, there are situations where you could use tailwind-merge but probably shouldn't. Think of tailwind-merge as an escape hatch rather than the primary tool to handle style variants.1

I think for now we should keep Prime on classnames, and add tailwind-merge to the places where we need to do overrides. I imagine then that we can look at our twMerge callsites and use that to refine the APIs of our style functions

[DTCurrie]

I am very much in favor of this change, but I think for now, we should avoid tailwind-merge. From their docs:

Reasons not to use it

Generally speaking, there are situations where you could use tailwind-merge but probably shouldn't. Think of tailwind-merge as an escape hatch rather than the primary tool to handle style variants.1

I think for now we should keep Prime on classnames, and add tailwind-merge to the places where we need to do overrides. I imagine then that we can look at our twMerge callsites and use that to refine the APIs of our style functions

I am okay with not doing it for now, but buttons and inputs are probably our most commonly overwritten components style-wise. It is hard to find all of the cases where we are using the important operator in use since the ! is so common.

I'd also argue that the usage here is kind of an escape hatch. Extra classes are the "I need to change the styles of this component, but it shouldn't require a new variant" lever we can pull. We could only use tailwind-merge for layering extra classes on to whatever variants we create with classnames, so it is only used when extra classes are applied.

[mcous]

I am okay with not doing it for now, but buttons and inputs are probably our most commonly overwritten components style-wise. It is hard to find all of the cases where we are using the important operator in use since the ! is so common.

Yeah definitely feeling the pain, and I think replacing ! usage with twMerge is a great idea, so I'm glad you've brought the library up. My only thought is that I think we could keep the escape hatch confined to our application code rather than our component library, so we can use it as a design signal to improve the component library

e.g. If we see <button class={twMerge(buttonStyles(), 'h-7.5')} >, we keep our fast velocity but also see that our buttonStyles function needs to grow a height option

[DTCurrie]

I am okay with not doing it for now, but buttons and inputs are probably our most commonly overwritten components style-wise. It is hard to find all of the cases where we are using the important operator in use since the ! is so common.

Yeah definitely feeling the pain, and I think replacing ! usage with twMerge is a great idea, so I'm glad you've brought the library up. My only thought is that I think we could keep the escape hatch confined to our application code rather than our component library, so we can use it as a design signal to improve the component library

e.g. If we see <button class={twMerge(buttonStyles(), 'h-7.5')} >, we keep our fast velocity but also see that our buttonStyles function needs to grow a height option

Yeah that's fair.

[micheal-parks]

it doesn't look like anything here is related to link, should we just call this .button? My assumption when looking at this name is that it's a special collection for link buttons, when it's just the primary button styles.