[Button] Replace booleans with new props

[aveline]

WHY are these changes introduced?

Fixes #10047

WHAT is this pull request doing?

• Adds variant and tone props
• Removes primary, outline, plain, monochrome, and primarySuccess booleans
• Updates props on usages of Button
• Renames primaryPlain to tertiary

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated the component's README.md with documentation changes
• Tophatted documentation changes in the style guide

[kyledurand]

This looks amazing Aveline, nice cleanup 👏

Can you add a major changelog entry and maybe also a TODO in the v11 to v12 migration guide?

[kyledurand]

I don't think we need the variant && or tone && for these. Or was it rendering something weird like false as a class?

[aveline]

Good catch, thanks!

[sam-b-rose]

Changes look awesome! I'm just still confused by all the variants 😅

[sam-b-rose]

I like the shift to variant and tone here. However, are we firm on the naming?

It seems we are mixing button shape and style with color in the variant prop. With this model, it seems like we could either move all style variations to variant (ex primaryDestructive, plainDestructive) or we keep them separate allow for combinations (ex: variant="plain" tone="primary") with the caveat that all combos have to work together.

Currently, I feel like I have to rely on documentation to understand which button style I'm going to get.

Guess I'm just still confused by all the variants 😓

cc @sarahill

[aveline]

Great points @samrose3. I agree the variants are less than ideal at the moment. The primaryPlain and monochromePlain are hacky workarounds for the state of things at the moment.

With tone I am trying to follow the API guidelines and my understanding is it will show up in other components like Banner so it would make sense to keep the values consistent between components. critical and success make sense applied to Banner but less sure about tone="primary" for Banner? primary seems more like a Button style to me.

[sarahill]

Agree the naming isn't intuitive to what they'll visually look like. How about something like the attached. I don't like plainMonochrome as it's describing a couple things but it's legacy and I believe we'll move away from it and plain as we continue to implement the Uplift.

Figma

[sam-b-rose]

This update makes more sense to me 👍 thanks for the additional context @aveline and for the variant updates @sarahill!

I am aligned with the above proposed variants and tone, just want to throw one final consideration and know your advise and feedback!

Would it be too complicated to remove monochrome and either use:

1. default the blue link color and use tone="netrual" for monochrome (follows API alignment tone proposal)
2. OR use a variant="link" that makes it clear the link color is used (tone has no effect)
3. OR default to monochrome color for plain and use tone="info" for a link style (not a fan of this one since info isn't really related here)

[aveline]

Great suggestions! I will update primaryPlain to be tertiary as @sarahill proposed.

I like those ideas @samrose3 if we were planning on keeping plainMonochrome but I would hesitate to introduce a neutral tone for a variant that is a hack and really should be removed.

These are the two instances of monochromePlain within Polaris

There are 36 more in web. It would be worth going through and cleaning that up at some point so we could remove the plainMonochrome variant altogether.

[sarahill]

I like those ideas @samrose3 if we were planning on keeping plainMonochrome but I would hesitate to introduce a neutral tone for a variant that is a hack and really should be removed.

➕ ☝️

This I would expect to be tertiary with inverted colors

This I would expect to be tertiary

Some of this is tough because with these updates we also have to account for the space they take up so I don't expect all these changes to happen now. Just some examples to Aveline's point of plainMonochrome isn't a long term pattern we want to promote.

[aveline]

This I would expect to be tertiary with inverted colors

So does this become a tertiaryInverted variant? Or how do we propose handling inverted? It's probably something that could apply to all variants?

This is something we need to put more thought into and until we have it a bit more solidified at the system level, plainMonochrome is the iterim state.

[sarahill]

This is something we need to put more thought into and until we have it a bit more solidified at the system level, plainMonochrome is the interim state.

Yea I'm tempted to say leave as is for now (use plainMonochrome) but we'll need to figure out the better solution. I don't know that adding explicit variants for inverse is great long-term. This feels like another place where component level theming could be beneficial but that feels like a follow up.

[aveline]

I agree