[Button] Improve the disclosure API for button, allowing user to specify the direction the disclosure icon faces #2431

qq99 · 2019-11-14T15:31:02Z

WHY are these changes introduced?

We have a need to use a Button to control the collapsed/expanded state of a Collapsible. When the collapsible is collapsed, we want to say Show more with downwards caret. When expanded, we want to say Show less with upwards caret.

The previous API is respected, so this is purely an opt-in enhancement

The icon will animate via CSS transition

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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

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

export function Playground() {
  const [expanded, setExpanded] = React.useState(false);

  return (
    <Page title="Playground">
      <Card sectioned>
        <Button plain>Button without `disclosure` prop</Button>
        <br />
        <Button>Button without `disclosure` prop</Button>
        <br />
        <Button plain disclosure>
          Button with `disclosure` prop
        </Button>
        <br />
        <Button plain disclosure="down">
          Button with `disclosure="down"` prop
        </Button>
        <br />
        <Button plain disclosure="up">
          Button with `disclosure="up"` prop
        </Button>
        <br />
        <Button
          plain
          disclosure={expanded ? 'up' : 'down'}
          onClick={() => setExpanded(!expanded)}
        >
          Button you can toggle down/up on click
        </Button>
        <br />
        <Button
          disclosure={expanded ? 'up' : 'down'}
          onClick={() => setExpanded(!expanded)}
        >
          Button you can toggle down/up on click
        </Button>
      </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

github-actions · 2019-11-14T15:33:02Z

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

Files modified	5
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)

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.

qq99 · 2019-11-14T15:45:57Z

I think percy bugged out? 🤔 Showing visual diff that I can't see locally

src/components/Button/Button.tsx

dleroux · 2019-11-14T16:09:43Z

The code looks good. I just question the animation. Since we don't have this type of animation today it sort of took me by surprise. Maybe @sarahill or @johanstromqvist can share their thoughts.

UNRELEASED.md

qq99 · 2019-11-14T16:16:45Z

I'm cool without the animation too, just have it instantaneous, whichever most people prefer

The implementation on today's OrderDetails page has no animation

dleroux · 2019-11-14T16:19:07Z

I think percy bugged out? 🤔 Showing visual diff that I can't see locally

It's fine, it's because you added a new example.

dleroux · 2019-11-14T16:20:13Z

I'm cool without the animation too, just have it instantaneous, whichever most people prefer

The implementation on today's OrderDetails page has no animation

I lean towards no animation, but let's see what the people I pinged think.

BPScott · 2019-11-14T16:38:06Z

For the upwards caret I think we should use the proper icon CaretUpMinor instead of rendering CaretDownMinor upside down

qq99 · 2019-11-14T16:57:38Z

I chose to use the same icon (but rotate it) for the CSS transitions, otherwise it would be impossible

If we want to go static and no animations, we could use CaretUpMinor

johanstromqvist · 2019-11-14T17:06:41Z

I don't have any strong opinions on the animation. If it does a good job for the user, I don't think it's a problem that it's novel.

Is it necessary? No I don't think so.
Is it an improvement from immediate change? Yes, I think it does add some context and responsiveness to the UI.

The animation probably looks more prominent isolated in the GIF, than it would when implemented. If the caret rotates together and in sync with the expand/collapse, I think the eye will be drawn to the content, not the caret.

UNRELEASED.md

qq99 · 2019-11-18T16:10:19Z

Good to ship? Anyone 2nd ✅ ?

brandon-yetman · 2019-11-19T20:09:47Z

Small example of the animation with popover content. Definitely echo @johanstromqvist that at least for me, the eye is drawn to the content and the animation is not as prominent. Makes the whole interaction feel more "fluid" in my opinion.

…ify the direction the disclosure icon faces, CSS transition to animate when the prop changes

tmlayton

For posterity: 👍

qq99 changed the title ~~[wip][Button] Improve the disclosure API for button, allowing user to spec…~~ [wip][Button] Improve the disclosure API for button, allowing user to specify the direction the disclosure icon faces Nov 14, 2019

qq99 force-pushed the button-disclosure-api-enhancement branch from cb31973 to 69b1ca2 Compare November 14, 2019 15:32

qq99 force-pushed the button-disclosure-api-enhancement branch from 69b1ca2 to 8391ad9 Compare November 14, 2019 15:40

qq99 marked this pull request as ready for review November 14, 2019 15:41

qq99 changed the title ~~[wip][Button] Improve the disclosure API for button, allowing user to specify the direction the disclosure icon faces~~ [Button] Improve the disclosure API for button, allowing user to specify the direction the disclosure icon faces Nov 14, 2019

qq99 requested review from dleroux and tmlayton November 14, 2019 15:43

dleroux reviewed Nov 14, 2019

View reviewed changes

src/components/Button/Button.tsx Outdated Show resolved Hide resolved

dleroux reviewed Nov 14, 2019

View reviewed changes

UNRELEASED.md Outdated Show resolved Hide resolved

qq99 force-pushed the button-disclosure-api-enhancement branch from 8391ad9 to c5de50b Compare November 14, 2019 16:18

dleroux self-requested a review November 14, 2019 17:08

dleroux approved these changes Nov 14, 2019

View reviewed changes

dleroux reviewed Nov 14, 2019

View reviewed changes

UNRELEASED.md Outdated Show resolved Hide resolved

qq99 force-pushed the button-disclosure-api-enhancement branch 2 times, most recently from c3e3373 to a3a2dd1 Compare November 18, 2019 16:34

brandon-yetman approved these changes Nov 19, 2019

View reviewed changes

qq99 force-pushed the button-disclosure-api-enhancement branch from a3a2dd1 to 1c6ffe6 Compare November 19, 2019 21:14

[Button] Improve the disclosure API for button, allowing user to spec…

d189453

…ify the direction the disclosure icon faces, CSS transition to animate when the prop changes

qq99 force-pushed the button-disclosure-api-enhancement branch from 1c6ffe6 to d189453 Compare November 19, 2019 21:32

qq99 merged commit f69365b into master Nov 19, 2019

qq99 deleted the button-disclosure-api-enhancement branch November 19, 2019 21:40

tmlayton reviewed Nov 19, 2019

View reviewed changes

dleroux temporarily deployed to production December 4, 2019 14:42 Inactive

[Button] Improve the disclosure API for button, allowing user to spec﻿﻿﻿ify the direction the disclosure icon faces #2431

[Button] Improve the disclosure API for button, allowing user to spec﻿﻿﻿ify the direction the disclosure icon faces #2431

Uh oh!

Conversation

qq99 commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Nov 14, 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)

Uh oh!

qq99 commented Nov 14, 2019

Uh oh!

Uh oh!

dleroux commented Nov 14, 2019

Uh oh!

Uh oh!

qq99 commented Nov 14, 2019

Uh oh!

dleroux commented Nov 14, 2019

Uh oh!

dleroux commented Nov 14, 2019

Uh oh!

BPScott commented Nov 14, 2019

Uh oh!

qq99 commented Nov 14, 2019

Uh oh!

johanstromqvist commented Nov 14, 2019

Uh oh!

Uh oh!

qq99 commented Nov 18, 2019

Uh oh!

brandon-yetman commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmlayton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[Button] Improve the disclosure API for button, allowing user to specify the direction the disclosure icon faces #2431

[Button] Improve the disclosure API for button, allowing user to specify the direction the disclosure icon faces #2431

qq99 commented Nov 14, 2019 •

edited

Loading

github-actions bot commented Nov 14, 2019 •

edited

Loading

brandon-yetman commented Nov 19, 2019 •

edited

Loading