🎨 [Button] Simplify code by utilizing more of UnstyledButton #3494

beefchimi · 2020-10-15T16:05:29Z

The goal of this PR is to consolidate concerns between UnstyledButton and Button.

Preamble

Not too long ago, I posted this message on Slack, which got answered by @dfmcphee - revealing that a UnstyledButton had just got merged to help folks utilize the mechanics of <Button /> while applying their own styling.

UnstyledButton will allow me to do away with a custom "action" component I crafted, and instead consume the Polaris component while passing my own styles + children. There will be some more work to be done however... and I am planning to propose a few API additions / component tweaks in the near future.

Before I do that, I thought it best to dig into what UnstyledButton + Button give me. I realized there was some repeated code between the two files, so I set about reducing the redundancy. While at it, I realized Button.test.tsx probably wasn't updated after UnstyledButton was introduced, so I went through the tests and did my best to clean them up.

Changes

While this PR is mostly just cleaning up some code, there are a few changes worth documenting:

The loading prop was not used by UnstyledButton, but some of the side-effects of that prop looked relevant to include. So, I've added loading to UnstyledButton so that both role and aria-busy can be toggled.
Button.tsx did not apply data-polaris-unstyled-button to the <a /> or UnstyledLink, but UnstyledButton did. I've removed some redundant code paths inside Button.tsx and the data-polaris-unstyled-button prop now gets applied.

Questions

(1) Do we really want [key: string]: any; as part of UnstyledButtonProps?

I realize this is done so we can pass any arbitrary prop, such as data-whatever.
But is this a bit too free-wheelin'?

(2) Should we pass onClick when url?

Does this make sense? What is the execution order?

(3) Why do we use toBeFalsey when a value is undefined? Should we not be more explicit?

(4) Does the way I am extending UnstyledButtonProps within Button.tsx affect how descriptions get surfaced in the styleguide?

~~(5) Do we always want to apply data-polaris-unstyled-button to the <a /> and UnstyledLink?`~~

~~Button.tsx did not use this attribute, so I am wondering if we should have a boolean prop for unstyledLink?~~

Playground

Copy + Paste

import React from 'react';

import {Page, Button, Stack} from '../src';

export function Playground() {
  // eslint-disable-next-line no-console
  const handleOnClick = () => console.log('onClick');

  return (
    <Page title="Playground">
      <Stack wrap>
        <Button onClick={handleOnClick}>Basic onClick</Button>
        <Button onClick={handleOnClick} disabled>
          Basic onClick and disabled
        </Button>
        <Button onClick={handleOnClick} loading>
          Basic onClick and loading
        </Button>
        <Button url="https://shopify.com" onClick={handleOnClick}>
          With URL
        </Button>
        <Button url="https://shopify.com" onClick={handleOnClick} disabled>
          With URL and disabled
        </Button>
        <Button url="https://shopify.com" onClick={handleOnClick} loading>
          With URL and loading
        </Button>
      </Stack>
    </Page>
  );
}

github-actions · 2020-10-15T16:07:47Z

🔴 This pull request modifies 8 files and might impact 116 other files. Because this is a larger than average splash zone for a change, remember to tophat areas that could be affected.

Details:

All files potentially affected (total: 116)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Button/Button.tsx (total: 54)

Files potentially affected (total: 54)

🧩 src/components/Button/tests/Button.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/ResourceItem/tests/ResourceItem.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/UnstyledButton/UnstyledButton.tsx (total: 56)

Files potentially affected (total: 56)

🧩 src/components/UnstyledButton/tests/UnstyledButton.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/types.ts (total: 112)

Files potentially affected (total: 112)

🧩 src/utilities/focus.ts (total: 70)

Files potentially affected (total: 70)

dfmcphee · 2020-10-23T15:07:21Z

src/components/UnstyledButton/UnstyledButton.tsx

+  /** Disables the button, disallowing merchant interaction */
+  disabled?: boolean;
+  /** Replaces button text with a spinner while a background action is being performed */
+  loading?: boolean;


Do you think this one makes sense here? I was debating this too. It does seem like it could be useful functionality to share, but also feels like a visual element that could behave strangely depending how the button is styled.

Never mind, I see now this only handles the a11y portion in UnstyledButton. That makes a lot of sense to me.

dfmcphee · 2020-10-23T15:30:46Z

src/components/Button/Button.tsx


-export interface ButtonProps {
+export interface ButtonProps
+  extends Pick<


So I just tested this in the styleguide and it doesn't show any of these props in the docs with how this is setup. I think the better way to do this would be to have a BaseButtonProps interface that both UnstyledButton and Button extend for their props similar to how we do the action types.

Incredible 🙏 thank you so much for testing this! I've gone ahead and pushed a new commit with your suggestion 😺

dfmcphee

This is looking really great now!

A few answers to your questions:

(1) Do we really want [key: string]: any; as part of UnstyledButtonProps?

I realize this is done so we can pass any arbitrary prop, such as data-whatever.

But is this a bit too free-wheelin'?

I think this is ok. We do the same thing on UnstyledLink as well. If you need to add a data attribute or aria attribute that we don't support, I think you should be able to with this component. Otherwise we run the risk of people still needing to use their own <button> still.

(2) Should we pass onClick when url?

Does this make sense? What is the execution order?

Ya, we need this for some link tracking and other events. onClick should take place before the navigation.

(3) Why do we use toBeFalsey when a value is undefined? Should we not be more explicit?

Great question. I actually just fixed a bunch of these in web too. It would be much better to be more explicit about these. We actually have a tslint rule against it there.

(4) Does the way I am extending UnstyledButtonProps within Button.tsx affect how descriptions get surfaced in the styleguide?

Just tested this in the style guide and it works great the way you have it setup now. 👍

* 🎨 [Button] Organize some props * 🚚 [Button] Move loading prop to UnstyledButton * 🎨 [Button] Extend UnstyledButtonProps * 👷 [ResourceItem] Increase testing criteria * 👷 [Button] Update tests * 👷 [UnstyledButton] Update tests * ⚗️ [Button] Simplify prop conditions * 🔥 [Button] Remove redundant deprecation warning * 🔥 [UnstyledButton] Remove unstyled-button prop * 📝 [UNRELEASED] Add notes * 🏷️ [BaseButton] New interface and type cleanup

beefchimi added the ⚗️ Development label Oct 15, 2020

beefchimi requested a review from dfmcphee October 15, 2020 16:05

beefchimi self-assigned this Oct 15, 2020

beefchimi force-pushed the unstyled-button-consistent-props branch 2 times, most recently from b560e2a to d7a631d Compare October 15, 2020 18:33

beefchimi requested review from chloerice and emma-boardman October 15, 2020 19:07

beefchimi force-pushed the unstyled-button-consistent-props branch 4 times, most recently from 56bd2d0 to d7f21dd Compare October 21, 2020 16:23

dfmcphee reviewed Oct 23, 2020

View reviewed changes

beefchimi force-pushed the unstyled-button-consistent-props branch 2 times, most recently from 3f4defc to e449f7e Compare October 23, 2020 18:13

beefchimi added 11 commits October 26, 2020 09:04

🎨 [Button] Organize some props

15348ff

🚚 [Button] Move loading prop to UnstyledButton

fa9362f

🎨 [Button] Extend UnstyledButtonProps

c13286b

👷 [ResourceItem] Increase testing criteria

85522c0

👷 [Button] Update tests

3d5ace1

👷 [UnstyledButton] Update tests

7242ecd

⚗️ [Button] Simplify prop conditions

f9c83d3

🔥 [Button] Remove redundant deprecation warning

18f614a

🔥 [UnstyledButton] Remove unstyled-button prop

3025ab7

📝 [UNRELEASED] Add notes

15aba55

🏷️ [BaseButton] New interface and type cleanup

1c7938e

beefchimi force-pushed the unstyled-button-consistent-props branch from 67b5196 to 1c7938e Compare October 26, 2020 13:05

dfmcphee approved these changes Oct 26, 2020

View reviewed changes

beefchimi merged commit 3702f56 into master Oct 26, 2020

beefchimi deleted the unstyled-button-consistent-props branch October 26, 2020 14:23

beefchimi mentioned this pull request Oct 30, 2020

✨ [Button] Add new role prop #3590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

🎨 [Button] Simplify code by utilizing more of UnstyledButton #3494

🎨 [Button] Simplify code by utilizing more of UnstyledButton #3494

Uh oh!

beefchimi commented Oct 15, 2020 •

edited

Loading

Uh oh!

github-actions bot commented Oct 15, 2020 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 54)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 56)

Files potentially affected (total: 0)

Files potentially affected (total: 112)

Files potentially affected (total: 70)

Uh oh!

dfmcphee Oct 23, 2020

Uh oh!

dfmcphee Oct 23, 2020

Uh oh!

dfmcphee Oct 23, 2020

Uh oh!

beefchimi Oct 23, 2020

Uh oh!

dfmcphee left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

🎨 [Button] Simplify code by utilizing more of UnstyledButton #3494

🎨 [Button] Simplify code by utilizing more of UnstyledButton #3494

Uh oh!

Conversation

beefchimi commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Preamble

Changes

Questions

Playground

Uh oh!

github-actions bot commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files potentially affected (total: 0)

Files potentially affected (total: 54)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 56)

Files potentially affected (total: 0)

Files potentially affected (total: 112)

Files potentially affected (total: 70)

Uh oh!

dfmcphee Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

dfmcphee Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

dfmcphee Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

beefchimi Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

dfmcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

beefchimi commented Oct 15, 2020 •

edited

Loading

github-actions bot commented Oct 15, 2020 •

edited

Loading