[Button] Add pressed state to #2148

AndrewMusgrave · 2019-09-16T20:34:49Z

WHY are these changes introduced?

Fixes #877

WHAT is this pull request doing?

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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

import React, {useState} from 'react';
import {SaveMinor} from '@shopify/polaris-icons';
import {Page, Card, Button, Checkbox, Stack} from '../src';

export function Playground() {
  const [selected, setSelected] = useState([
    true,
    true,
    true,
    true,
    true,
    true,
    true,
    true,
    true,
    true,
  ]);

  const handleChange = (idx) => (val) =>
    setSelected((selected) => {
      const arr = [...selected];
      arr[idx] = val;
      return arr;
    });

  return (
    <Page title="Playground">
      <Card title="Primary">
        <Card.Section>
          <Stack>
            <Button primary pressed={selected[0]}>
              Primary - Pressed
            </Button>
            <Checkbox
              label=""
              checked={selected[0]}
              onChange={handleChange(0)}
            />
          </Stack>
        </Card.Section>
      </Card>

      <Card title="Destructive">
        <Card.Section>
          <Stack>
            <Button destructive pressed={selected[1]}>
              Destructive - Pressed
            </Button>
            <Checkbox
              label=""
              checked={selected[1]}
              onChange={handleChange(1)}
            />
          </Stack>
        </Card.Section>
      </Card>

      <Card title="Destructive - outline">
        <Card.Section>
          <Stack>
            <Button destructive outline pressed={selected[2]}>
              Destructive - Outline - Pressed
            </Button>
            <Checkbox
              label=""
              checked={selected[2]}
              onChange={handleChange(2)}
            />
          </Stack>
        </Card.Section>
      </Card>

      <Card title="Outline">
        <Card.Section>
          <Stack>
            <Button pressed={selected[3]} outline>
              Outline - Pressed
            </Button>
            <Checkbox
              label=""
              checked={selected[3]}
              onChange={handleChange(3)}
            />
          </Stack>
        </Card.Section>
        <Card.Section>
          <Stack>
            <Button pressed={selected[4]} outline monochrome>
              Outline - Pressed - Monochrome
            </Button>
            <Checkbox
              label=""
              checked={selected[4]}
              onChange={handleChange(4)}
            />
          </Stack>
        </Card.Section>
      </Card>

      <Card title="Plain">
        <Card.Section>
          <Stack>
            <Button pressed={selected[5]} plain>
              Outline - Plain
            </Button>
            <Checkbox
              label=""
              checked={selected[5]}
              onChange={handleChange(5)}
            />
          </Stack>
        </Card.Section>
        <Card.Section>
          <Stack>
            <Button pressed={selected[6]} plain monochrome>
              Outline - Plain - Monochrome
            </Button>
            <Checkbox
              label=""
              checked={selected[6]}
              onChange={handleChange(6)}
            />
          </Stack>
        </Card.Section>
      </Card>

      <Card title="Regular Button">
        <Card.Section>
          <Stack>
            <Button pressed={selected[7]}>Regular - pressed</Button>
            <Checkbox
              label=""
              checked={selected[7]}
              onChange={handleChange(7)}
            />
          </Stack>
        </Card.Section>
      </Card>

      <Card title="With Icon">
        <Card.Section>
          <Stack>
            <Button icon={SaveMinor} pressed={selected[8]} />
            <Checkbox
              label=""
              checked={selected[8]}
              onChange={handleChange(8)}
            />
          </Stack>
        </Card.Section>
      </Card>
    </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
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

AndrewMusgrave · 2019-09-24T20:19:42Z

Slight delay - Waiting to pair with a designer to hammer out visual states

AndrewMusgrave · 2019-09-30T19:12:51Z

src/components/Button/Button.tsx

  ariaExpanded?: boolean;
-  /** Tells screen reader the element is pressed */
+  /**
+   * @deprecated As of release 4.X.X, replaced by {@link https://polaris.shopify.com/components/structure/page#props-pressed}


TODO: Edit this while merging

AndrewMusgrave · 2019-09-30T19:30:23Z

src/styles/shared/_buttons.scss

-@mixin button-outline($outline-color) {
-  background: transparent;
+@mixin button-filled-pressed($button-color, $focus-color) {
+  background: linear-gradient(


Grabbed styles from ToggleButton in web

tmlayton · 2019-10-03T01:48:44Z

Hey @AndrewMusgrave, can we hold off on this until after we make the changes with CSS custom properties and theming in the next few months? Otherwise, let’s pair on reworking this mixin to avoid color manipulation as it makes the code for backwards compatibility kind of gross. Thanks!

danrosenthal

Can you add an example so folks can easily tophat this change and see it in use in our documentation once it's released?

BPScott · 2019-10-03T17:28:30Z

I got tagged as this is modifying the public sass API. Eventually we want to stop touching that but we've not got the time to bash out that plan. The change to that looks fine for now.

Removing myself from review as I haven't got the time to look at the rest of this

AndrewMusgrave · 2019-10-03T21:10:54Z

Hey AndrewMusgrave, can we hold off on this until after we make the changes with CSS custom properties and theming in the next few months? Otherwise, let’s pair on reworking this mixin to avoid color manipulation as it makes the code for backwards compatibility kind of gross. Thanks!

A few months is a little bit longer to let this go stale, I'll remove all the sass manipulation 😄

Can you add an example so folks can easily tophat this change and see it in use in our documentation once it's released?

Good call, I'll include an example. There are a lot of button variants though. The PR description has a complex playground to see the pressed states for 🎩

@mirualves Want to spend a few minutes going over the visuals Monday for a final check?

github-actions · 2019-10-03T21:32:31Z

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified	6
Files potentially affected	54

Details

All files potentially affected (total: 54)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/Button/Button.scss (total: 54)

Files potentially affected (total: 54)

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

Files potentially affected (total: 53)

📄 src/components/Button/README.md (total: 0)

Files potentially affected (total: 0)

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

Files potentially affected (total: 0)

🎨 src/styles/shared/_buttons.scss (total: 0)

Files potentially affected (total: 0)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

tmlayton

Didn’t do a full review, but giving an approval for the sass variable approach, thanks for doing that!

src/components/Button/Button.scss

danrosenthal

Thanks for the color notes!

Make sure there's a test passing the pressed property and this should be good to go.

AndrewMusgrave · 2019-10-10T18:48:14Z

Make sure there's a test passing the pressed property and this should be good to go.

I added a few tests. Historically we haven't tested visuals, is this something we're starting to do?

@danrosenthal I moved the new mixings into Button.scss so they're not exposed

danrosenthal · 2019-10-10T19:20:15Z

I added a few tests. Historically we haven't tested visuals, is this something we're starting to do?

It's not so much about testing visuals, as it is just having coverage on all code paths. I'd even be fine with adding pressed to an existing test just to get that code path to execute. The presence of the classNames is less interesting, and can be covered by VRT. Passing a prop just to ensure nothing throws feels like a perfectly valid way to test props whose only impact is classNames.

@danrosenthal I moved the new mixings into Button.scss so they're not exposed

Fantastic!

dleroux

🎉 Small suggestion on the api documentation, but looks good!

dleroux · 2019-10-15T12:53:31Z

src/components/Button/Button.tsx

  textAlign?: TextAlign;
  /** Gives the button a subtle alternative to the default button styling, appropriate for certain backdrops */
  outline?: boolean;
+  /** Demonstrates a pressed state */


Suggested change

/** Demonstrates a pressed state */

/** Gives the button the appearance of being pressed */

AndrewMusgrave mentioned this pull request Sep 18, 2019

[Button] Add “pressed” state #877

Closed

AndrewMusgrave force-pushed the add-pressed-button-state branch from 0c5d80a to 73a0144 Compare September 20, 2019 17:53

AndrewMusgrave requested a review from mirualves September 27, 2019 18:30

mirualves approved these changes Sep 27, 2019

View reviewed changes

AndrewMusgrave commented Sep 30, 2019

View reviewed changes

AndrewMusgrave force-pushed the add-pressed-button-state branch from 5bcf841 to 501480f Compare September 30, 2019 19:28

AndrewMusgrave commented Sep 30, 2019

View reviewed changes

AndrewMusgrave force-pushed the add-pressed-button-state branch 2 times, most recently from cd00ac5 to cde0f3b Compare September 30, 2019 19:58

AndrewMusgrave changed the title ~~[WIP][Button] Add pressed state to~~ [Button] Add pressed state to Sep 30, 2019

AndrewMusgrave marked this pull request as ready for review September 30, 2019 20:02

AndrewMusgrave requested review from BPScott, danrosenthal and tmlayton as code owners September 30, 2019 20:02

AndrewMusgrave requested a review from dleroux September 30, 2019 20:02

danrosenthal reviewed Oct 3, 2019

View reviewed changes

BPScott removed their request for review October 3, 2019 17:26

AndrewMusgrave force-pushed the add-pressed-button-state branch from cde0f3b to 2e43b2c Compare October 3, 2019 21:29

tmlayton approved these changes Oct 3, 2019

View reviewed changes

danrosenthal reviewed Oct 7, 2019

View reviewed changes

src/components/Button/Button.scss Show resolved Hide resolved

AndrewMusgrave requested a review from danrosenthal October 8, 2019 17:47

danrosenthal approved these changes Oct 8, 2019

View reviewed changes

AndrewMusgrave force-pushed the add-pressed-button-state branch from 4d1305c to 66aa609 Compare October 10, 2019 16:56

dleroux approved these changes Oct 15, 2019

View reviewed changes

dleroux reviewed Oct 15, 2019

View reviewed changes

dleroux mentioned this pull request Oct 15, 2019

[Button] render button in focused state #2280

Closed

AndrewMusgrave added 2 commits October 16, 2019 11:37

✨ Added pressed state to Button

76ea1c5

📝 changelog

6e2e4da

AndrewMusgrave force-pushed the add-pressed-button-state branch from 8adbb3b to 6e2e4da Compare October 16, 2019 15:38

📝 Address doc suggestion

133c466

AndrewMusgrave merged commit aead592 into master Oct 16, 2019

AndrewMusgrave deleted the add-pressed-button-state branch October 16, 2019 15:48

LauraAubin temporarily deployed to production October 22, 2019 17:39 Inactive

	/** Demonstrates a pressed state */
	/** Gives the button the appearance of being pressed */

[Button] Add pressed state to #2148

[Button] Add pressed state to #2148

Uh oh!

Conversation

AndrewMusgrave commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

AndrewMusgrave commented Sep 24, 2019

Uh oh!

AndrewMusgrave Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

tmlayton commented Oct 3, 2019

Uh oh!

danrosenthal left a comment

Choose a reason for hiding this comment

Uh oh!

BPScott commented Oct 3, 2019

Uh oh!

AndrewMusgrave commented Oct 3, 2019

Uh oh!

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

Uh oh!

Details

Files potentially affected (total: 0)

Files potentially affected (total: 54)

Files potentially affected (total: 53)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Uh oh!

tmlayton left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danrosenthal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave commented Oct 10, 2019

Uh oh!

danrosenthal commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dleroux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleroux Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AndrewMusgrave commented Sep 16, 2019 •

edited

Loading

github-actions bot commented Oct 3, 2019 •

edited

Loading

tmlayton left a comment •

edited

Loading

danrosenthal left a comment •

edited

Loading

danrosenthal commented Oct 10, 2019 •

edited

Loading

dleroux left a comment •

edited

Loading