Button: deprecate `isPrimary`, `isSecondary`, `isTertiary` and `isLink` props in favour of `variant` prop #31713

ciampo · 2021-05-11T15:08:57Z

Description

This PR takes inspiration from previous PRs #29990 and #31297

Expose a new variant prop (i.e. variant="primary|secondary|tertiary|link")
Do not mark the isPrimary, isSecondary, isTertiary and isLink props as deprecated yet, but add logic to the <Button /> component to translate their values into the variant prop
Update docs, changelog, unit tests and Storybook Button stories
Add unit tests to verify that the above-mentioned props are effectively marked as deprecated, while the expected classes are still being applied
Update all of the usages in the repo to utilise the new variant prop instead of the deprecated props (necessary to have all tests passing)

Part of the next steps highlighted in #30503

Testing instructions

Run Gutenberg and make sure that Buttons still work as expected
All tests are passing
Run Button unit tests: npm run test-unit packages/components/src/button
Run local storybook npm run storybook:dev and compare the Button component against what's currently in production
Search in the repo's codebase for the isPrimary, isSecondary, isTertiary and isLink props and make sure they have all been replaced with the new variant prop

Screenshots

N/A

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
N/A I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

packages/components/src/button/index.js

packages/components/src/button/README.md

kevin940726

I like this a lot!

packages/components/src/button/test/deprecated-props.js

packages/components/src/placeholder/style.scss

ciampo · 2021-05-14T10:41:52Z

Rebased on top of latest trunk, and removed the official deprecation notices in 8344c80.

The docs don't mention the isPrimary, isSecondary, isTertiary and isLink props anymore, in favor of the new variant prop

sarayourfriend

Once this is rebased it LGTM, nice work.

@youknowriad any ideas when we would be able to actually add deprecation warnings for the old props?

youknowriad · 2021-05-17T16:30:55Z

Right after 5.8 beta freeze would work for me. I guess that means next week :)

ciampo · 2021-05-20T11:32:10Z

packages/edit-post/src/components/header/template-title/delete-template.js

@@ -46,8 +46,7 @@ export default function DeleteTemplate() {
 		<MenuGroup className="edit-post-template-top-area__second-menu-group">
 			<MenuItem
 				isDestructive
-				isTertiary
-				isLink
+				variant="tertiary"


Hey @jorgefilipecosta , we are currently in the process of replacing isTertiary and isLink with a new variant prop — which also means that a button can be assigned to only one variant at the time.

Does this fit your needs? Or did you need this button to be at the same tertiary AND marked as a link?
We're also considering keeping isLink (see #31713 (comment))

The design is changed a little bit. I think it is ok (provided we correct a small pixel alignment between the button and the input as a follow up):
new:

old:

But I'm going to cc: @jameskoster to double-check if we can do this UI change.

I could also change it to variant="link" if that works better

Should be fine, I actually have a PR open to adjust that popover a bit. There it's only using isSecondary and isDestructive which I assume will work fine with the new system?

There it's only using isSecondary and isDestructive which I assume will work fine with the new system?

That's not going to be an issue. isSecondary will become variant="secondary" and isDestructive can stay as-is

mtias · 2021-05-20T21:25:40Z

packages/block-directory/src/plugins/get-install-missing/index.js

@@ -88,7 +88,7 @@ const ModifiedWarning = ( { originalBlock, ...props } ) => {
 			originalBlock.title || originalName
 		);
 		actions.push(
-			<Button key="convert" onClick={ convertToHTML } isLink>
+			<Button key="convert" onClick={ convertToHTML } variant="link">


Wonder if it's slightly odd to see link as a variant. Would it ever make sense to have a "primary link" or a "secondary link"? Should externalLink be considered a variant as well (it exists as a separate thin now)?

variant="link" doesn't determine whether the component renders an anchor tag, that's determined by the href prop's presence.

Furthermore, the link variant cannot be mixed with the other three variants for a good looking experience. These are the three variants together with link:

They all say "tertiary" button but they're the mixes of link with tertiary, secondary and primary respectively.

If it's determined by href why do we need it as a variant?

link is a variant of the style of the button, not the functionality. Passing href changes the functionality. You could, for example, have a button that looks like a link by passing <Button onClick={() => alert('hello!')} variant="link">Hello!</Button>

We should probably rename it to text or flat though. material-ui is a good source of inspiration for this. (not necessary related to this PR though)

I'd expect text to look most like the variantless button though, i.e., without the underline. 🤷‍♀️ I'm fine renaming it to whatever makes the most sense to people but I don't think we'll find something perfect. Our variants aren't as "varied" as material's. We really just have the five: no variant, primary, secondary, tertiary, and link. And they all or remove the outline/background arbitrarily (link, incidentally, is probably the easiest to guess what it does currently aside from the confusion around functionality).

I agree. I think it would be best to merge this change (i.e. introducing variant as a preferred alternative to the overlapping is props) and then we could potentially reconsider renaming or changing variants separately. Don't want to keep this PR open any longer because it touches so many usages of Button that it needs constant rebasing, etc.

It's weird to use "link" to refer to an appearance characteristic but we can revisit this separately as we should also clarify the design expectations.

mtias · 2021-05-20T21:27:38Z

I like this.

Do not mark the isPrimary, isSecondary, isTertiary and isLink props as deprecated yet, but add logic to the component to translate their values into the variant prop

This sounds like a good approach to me. I think we might consider soft deprecating — just converting internally but not warning — given how widespread the use of this component is.

…iant prop

…/deprecated`, move new tests into one file

…precated

Lovor01 · 2021-10-25T15:59:14Z

This change is introduced in the docs too early. WordPress 5.8.1 still uses button with is**** attributes, and these deprecations (will still work in future versions - I inspected code) are not mentioned at all in the docs for Button component

ciampo · 2021-11-09T11:37:03Z

Hey @Lovor01!

This change is introduced in the docs too early [...] these deprecations are not mentioned at all in the docs for Button component

This deprecation was mentioned in the changelog.

Regarding the mention on the Button docs, we are on purpose not mentioning it. We just removed the props from the docs (while still supporting them in code), in order to encourage the usage of the new variant prop without introducing a breaking change. You can read more about this approach in the contributing guidelines

WordPress 5.8.1 still uses button with is**** attributes

Could you be more precise about which Buttons in the codebase you are referring to?

We use variant instead, since WordPress/gutenberg#31713

ciampo mentioned this pull request May 11, 2021

Fully integrating vanilla Emotion as the canonical style system for CSS-in-JS in Gutenberg #30503

Closed

ciampo changed the title ~~Button: deprecate isPrimary, isSecondary and isTertiary props in favour of variant prop~~ Button: deprecate isPrimary, isSecondary, isTertiary and isLink props in favour of variant prop May 11, 2021

ciampo commented May 11, 2021

View reviewed changes

packages/components/src/button/index.js Outdated Show resolved Hide resolved

ciampo commented May 11, 2021

View reviewed changes

packages/components/src/button/index.js Outdated Show resolved Hide resolved

ciampo commented May 11, 2021

View reviewed changes

packages/components/src/button/README.md Show resolved Hide resolved

ciampo marked this pull request as ready for review May 12, 2021 10:20

ciampo requested review from adamziel, ajitbohra, chrisvanpatten, draganescu, ellatrix, kevin940726, nerrad, noisysocks, ntwb, talldan and tellthemachines as code owners May 12, 2021 10:20

kevin940726 reviewed May 14, 2021

View reviewed changes

packages/components/src/button/test/deprecated-props.js Outdated Show resolved Hide resolved

packages/components/src/button/test/deprecated-props.js Outdated Show resolved Hide resolved

packages/components/src/placeholder/style.scss Outdated Show resolved Hide resolved

ciampo requested a review from getdave as a code owner May 14, 2021 08:22

sarayourfriend approved these changes May 17, 2021

View reviewed changes

ciampo commented May 20, 2021

View reviewed changes

ciampo self-assigned this May 20, 2021

ciampo added [Feature] Component System WordPress component system [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality labels May 20, 2021

mtias reviewed May 20, 2021

View reviewed changes

Deprecate isPrimary, isSecondary and isTertiary props in favor of var…

8082eeb

…iant prop

ciampo added 16 commits May 26, 2021 12:55

Update button docs

f1163ec

Update Button variant in Storybook

2d2e532

Update tests

b744bc0

changelog

856c632

Swap order of isSecondary / isTertiary deprecation

67032a6

Depracate isLink prop too

ddf31eb

Spacing

a0ed04d

Update changelog

4253926

Update example in README

a4601e0

Add unit test for deprecated props

130ad24

Use expect( console ).toHaveWarned() instead of mocking `@wordpress…

e6e0e5a

…/deprecated`, move new tests into one file

Revert auto-formatting

36ae2c6

Move SCSS comment above selector

9d7d2db

Do not mark isPrimary, isSecondary, isTertiary and isLink props as de…

c030431

…precated

Remove TODO comment

973feec

Better wording for the notice component README

4bfd1d4

ciampo merged commit 9df736c into WordPress:trunk May 26, 2021

ciampo deleted the feat/components-button-variant-deprecation branch May 26, 2021 14:36

github-actions bot added this to the Gutenberg 10.8 milestone May 26, 2021

This was referenced Feb 15, 2022

Some props in @wordpress/components <Button> are deprecated woocommerce/woocommerce-admin#8313

Closed

Replace deprecated isPrimary, isSecondary, isTertiary, isLink Button props with variant woocommerce/woocommerce-admin#8340

Closed

jeherve mentioned this pull request Mar 4, 2022

Button Component: update to use non-deprecated attributes Automattic/jetpack#23242

Merged

jeherve added a commit to Automattic/jetpack that referenced this pull request Mar 4, 2022

Replace all isPrimary references

1346801

We use variant instead, since WordPress/gutenberg#31713

chad1008 mentioned this pull request Jan 19, 2023

Notice: refactor to TypeScript #47118

Merged

nielslange mentioned this pull request Nov 21, 2023

Replace deprecated button props isPrimary, isSecondary, isTertiary and isLink woocommerce/woocommerce-blocks#11855

Closed

tyxla mentioned this pull request Mar 15, 2024

Button: Keep deprecated props in type definitions #59913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Button: deprecate `isPrimary`, `isSecondary`, `isTertiary` and `isLink` props in favour of `variant` prop #31713

Button: deprecate `isPrimary`, `isSecondary`, `isTertiary` and `isLink` props in favour of `variant` prop #31713

ciampo commented May 11, 2021 •

edited

Loading

kevin940726 left a comment

ciampo commented May 14, 2021 •

edited

Loading

sarayourfriend left a comment

youknowriad commented May 17, 2021

ciampo May 20, 2021 •

edited

Loading

jorgefilipecosta May 24, 2021

ciampo May 24, 2021

jameskoster May 24, 2021

ciampo May 24, 2021

mtias May 20, 2021

sarayourfriend May 21, 2021 •

edited

Loading

mtias May 21, 2021

sarayourfriend May 21, 2021 •

edited

Loading

kevin940726 May 24, 2021 •

edited

Loading

sarayourfriend May 24, 2021

griffbrad May 24, 2021

mtias May 26, 2021

mtias commented May 20, 2021

Lovor01 commented Oct 25, 2021

ciampo commented Nov 9, 2021

Button: deprecate isPrimary, isSecondary, isTertiary and isLink props in favour of variant prop #31713

Button: deprecate isPrimary, isSecondary, isTertiary and isLink props in favour of variant prop #31713

Conversation

ciampo commented May 11, 2021 • edited Loading

Description

Testing instructions

Screenshots

Types of changes

Checklist:

kevin940726 left a comment

Choose a reason for hiding this comment

ciampo commented May 14, 2021 • edited Loading

sarayourfriend left a comment

Choose a reason for hiding this comment

youknowriad commented May 17, 2021

ciampo May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend May 21, 2021 • edited Loading

Choose a reason for hiding this comment

kevin940726 May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtias commented May 20, 2021

Lovor01 commented Oct 25, 2021

ciampo commented Nov 9, 2021

Button: deprecate `isPrimary`, `isSecondary`, `isTertiary` and `isLink` props in favour of `variant` prop #31713

Button: deprecate `isPrimary`, `isSecondary`, `isTertiary` and `isLink` props in favour of `variant` prop #31713

ciampo commented May 11, 2021 •

edited

Loading

ciampo commented May 14, 2021 •

edited

Loading

ciampo May 20, 2021 •

edited

Loading

sarayourfriend May 21, 2021 •

edited

Loading

sarayourfriend May 21, 2021 •

edited

Loading

kevin940726 May 24, 2021 •

edited

Loading