feat(@clayui/link): add ability to style link like a button

[bryceosterhaus]

fixes #3301

Here is a POC of adding ClayLink to Button so that we can render both links as buttons and buttons as links.

Any concerns with this?

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8514628:

Sandbox	Source
laughing-surf-v0g9h	Configuration
interesting-brahmagupta-nkr17	Configuration

[kresimir-coko]

Hey @bryceosterhaus, wouldn't adding support for a button value of displayType in ClayLink be more consistent?

I wonder why you went with this approach, putting both in ClayButton, adding a dependency to ClayButton that is only necessary when it's used as a link.

While discussing this yesterday with @jbalsas I envisioned just adding the displayType="button" and then rendering a button within the ClayLink component.

I'm interested in your thinking while going about doing it this way @bryceosterhaus.

[jbalsas]

This does feel weirdly unsemantic to me as well.

<ClayButton href="..." /> when the HTML <button> tag does not have that attribute...

It does hint to this being something else, though...

The key is, what is the developer expectation? What's more important, the implementation or the design? Does the user know she needs a link <a> rather than a button <button>? (it would appear so, being a frontend and using href ...) or does she know she needs a button looking at the design and doesn't really care how it gets implemented underneath? (again... having to use href kinda trumps that assumption).

I'm torn here and don't have a clear answer 🤔

[kresimir-coko]

In my opinion I think the functionality should be what drives the decision when in the process of using the component. For example:

1. I want link functionality so I'll use ClayLink
   1.1. I expect one (or combination) of the props to allow me to make this link look like a button
2. I want button functionality so I'll use ClayButton
   2.1. I expect one (or combination) of the props to allow me to make this button look like a link

This is keeping in mind that in any circumstance, at least in a modern stack, the user can just style their buttons and links however they want.
I don't think we should make our implementation so it prioritizes some specific design need first, but functionality and consistency instead

[wincent]

when the HTML <button> tag does not have that attribute

That seems like a bit of a problem. I would expect React to warn about us doing that in development mode (maybe it doesn't though 🤷 ). So, independently of the design question we might want to replace the ContainerElement ternary with an if/else that actually renders a button or an a (literally).

On the design question, I don't know if it is useful, but my intuition seems to match up with what @kresimir-coko said. I would intuitively expect @clayui/link to render an a, and @clayui/button to render a button, and if I want them to look like the other I'd expect to be able to change their appearance (via a class, or a prop that sets a class or at most tweaks the structure of the markup), but I wouldn't expect that to make the actual element rendered to change.

[jbalsas]

[...] we might want to replace the ContainerElement ternary with an if/else that actually renders a button or an a (literally).

That's exactly what this one does, actually.

when the HTML tag does not have that attribute...

I meant that since button doesn't have the href attribute, I wouldn't expect ClayButton to take it and make it behave as something else... but as I said, I can see both sides of this coin.

[wincent]

[...] we might want to replace the ContainerElement ternary with an if/else that actually renders a button or an a (literally).

That's exactly what this one does, actually.

I see I didn't make myself clear. I was saying that I'd expect React to warn in development because of the ContainerElement trick in this PR, and I was suggesting instead the "dumb" version that does:

if (href) {
    return <a ...>;
} else {
    return <button ...>;
}

[bryceosterhaus]

I was also a little uneasy about putting ClayLink into Button for semantics. The main reason I went about it this was is because I didn't want to duplicate all the className logic that button uses....

'btn-block': block,
'btn-monospaced': monospaced,
'btn-outline-borderless': borderless,
'btn-sm': small,
[`btn-${displayType}`]: displayType && !outline && !borderless,
[`btn-outline-${displayType}`]: displayType && (outline || borderless),

I'm imaging that once we add the ability to make a link look like a button, we also need to add all the button variants. Maybe duplication of this isn't bad though.

Any idea how common this pattern is of making links look like buttons? The design of it seems off, but maybe I just can't think of clear examples. If its not common or if it's an anti-pattern in design, maybe we just recommend adding the appropriate classes.

[jbalsas]

Any idea how common this pattern is of making links look like buttons?

There's a saying in spanish that's perfect for this. "Para muestra, un botón"...

Apparently "The proof is in the pudding" is the proper translation for this 🤷

[jbalsas]

[...] I didn't want to duplicate all the className logic that button uses [...]

That's a good argument... this is a common enough pattern that should be easy to get right and not overcomplicated exposing internal CSS classes... but again, maybe copy-pasting that logic isn't the worse that can happen... 🤔

[pat270]

should change achor to anchor?

[bryceosterhaus]

@jbalsas from that picture you linked, only one of those really looks like a button to me, "New Entry". For the rest of those, are we expecting people to use button styles for those? I would think those would just be links around an icon or text. I think both APIs seem odd to me when I look at it though...

<ClayButton href="/goes/somewhere">Click me</ClayButton>

<ClayLink button href="/goes/somewhere" displayType="primary">Click me</ClayLink>

ClayLink would likely have to duplicate the same props as button as well, not just the class name logic.

If I had to choose one, I think I would lean to using href in ClayButton. But that could also be that my brain reads the button/link jsx more as "what is this component displaying" vs "what is this component doing".

tldr;
I have no idea

[jbalsas]

[...] only one of those really looks like a button to me [...]

Well... that's because they're borderless buttons. They do have all the proper states when you hover and focus over them, though, different from a Link 🤷

[bryceosterhaus]

@jbalsas whats your preference for where this logic lies? I don't feel strongly enough to really push for one side.

If I had to choose, I would go with Button w/ href, but I am open to Link w/ button props if people feel strongly about that.

[jbalsas]

@jbalsas whats your preference for where this logic lies? I don't feel strongly enough to really push for one side.

I honestly have no idea what's the right call...

If you're asking for preference, I'd prefer to put this in link, as in I prefer our devs to think "I want a link that looks like a button"...

The design of it seems off, but maybe I just can't think of clear examples. If its not common or if it's an anti-pattern in design [...]

I think this is kinda the right train of thought... links are links and buttons are buttons for reasons and backed by years of research. While we want to allow deviating from that path... we don't necessarily need to make it super easy (just convenient enough). So, adding some friction here in terms of a developer having to translate from "visual" to "implementation" should be okayish...

That's my thinking, but I can be persuaded

[bryceosterhaus]

@jbalsas makes sense to me. I like the idea of adding some friction to it, which I think having button props on a Link makes sense. We might even limit the props to only a few and require classes for more nuanced versions of button.

I'll update this PR today

[bryceosterhaus]

just updated

[bryceosterhaus]

note, I had to change the tsconfig target to allow for optional chaining syntax. This is probably the target we should have had this entire tiem

[wincent]

This is probably the target we should have had this entire tiem

😱

[jbalsas]

LGTM!

[wincent]

It's a shame you need the ?. here, because I would have thought that TS would have been able to narrow the type; eg.

• At top, we know button is boolean, {} or undefined.
• After if, we know it is true or {}.
• After button = reassignment, we know it is {}

It seems that if you assign it to a variable then it can narrow it for sure (eg. playground):

function x(
    button?:
        | boolean
        | {
              block?: boolean,
              monospaced?: boolean,
              small?: boolean,
          }
) {
    if (button) {
        /*
         * Type of `narrowed` inferred here is:
         *
         *     {
         *         block?: boolean | undefined;
         *         monospaced?: boolean | undefined;
         *         small?: boolean | undefined;
         *     }
         */
        const narrowed = button === true ? {} : button;
    }
}

[bryceosterhaus]

@wincent yeah I think I initially tried to use the optional chaining and was still getting errors, so then I added the assignment and it was fine. But you are right, the chaining was redundant and not needed, I just removed it in the latest commit.