[Button] add support for split disclosure button #2329

chloerice · 2019-10-18T20:24:28Z

WHY are these changes introduced?

There is currently no way to expose merchants to related primary actions in the ContextualSaveBar or Page Header because they only support a single primary action. For example, if the primary action is to save a resource like an order, but you can also save the order as a draft.

WHAT is this pull request doing?

This PR adds support for a split button variant. This is the same UX as the split button you are probably familiar with in Gmail. It renders a disclosure button connected right which toggles a popover action list with related actions.

Split button	Split button with disclosure active

Click to view other UX considered

Another possible UX could be that the action list acts as a way to choose which action the primary button renders. You may be familiar with that type of split button if you've ever created a draft PR here on GitHub. The currently selected action has a selected state in the action list. Clicking a related action closes the popover and updates the primary action to be that clicked action.

cc @ShaniqueS @mirualves

Initial primary action	Related primary action selected

	Split button variants
Current
Light mode
Dark mode

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

To tophat in Chroma deployment

Click to view the split button example in a new tab

To tophat locally

git checkout button-connectedSegment && dev up && yarn dev
- A new tab or window with the Storybook examples will open automatically
Open polaris-react in your text editor and copy/paste the Playground code collapsed below into /playground/Playground.tsx
- Click "Playground" in the left navigation of the Storybook to view the playground

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

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

export function Playground() {
  return (
    <Page title="Playground">
      <Card sectioned>
        <ButtonGroup>
          <Button
            onClick={() => console.log('Draft purchase order saved')}
            primary
            connectedDisclosure={{
              actions: [
                {
                  content: 'Save and mark as ordered',
                  onAction: () => console.log('PO saved and marked as ordered'),
                },
              ],
            }}
          >
            Primary
          </Button>
          <Button
            onClick={() => console.log('Draft purchase order saved')}
            connectedDisclosure={{
              actions: [
                {
                  content: 'Save and mark as ordered',
                  onAction: () => console.log('PO saved and marked as ordered'),
                },
              ],
            }}
          >
            Basic
          </Button>
          <Button
            outline
            onClick={() => console.log('Draft purchase order saved')}
            connectedDisclosure={{
              actions: [
                {
                  content: 'Save and mark as ordered',
                  onAction: () => console.log('PO saved and marked as ordered'),
                },
              ],
            }}
          >
            Outline
          </Button>
          <Button
            destructive
            onClick={() => console.log('Draft purchase order saved')}
            connectedDisclosure={{
              actions: [
                {
                  destructive: true,
                  content: 'Save and mark as ordered',
                  onAction: () => console.log('PO saved and marked as ordered'),
                },
              ],
            }}
          >
            Destructive
          </Button>
          <Button
            outline
            destructive
            onClick={() => console.log('Draft purchase order saved')}
            connectedDisclosure={{
              actions: [
                {
                  destructive: true,
                  content: 'Save and mark as ordered',
                  onAction: () => console.log('PO saved and marked as ordered'),
                },
              ],
            }}
          >
            Destructive
          </Button>
        </ButtonGroup>
      </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-10-23T22:31:24Z

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

Files modified	29
Files potentially affected	135

Details

All files potentially affected (total: 135)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

📄 locales/cs.json (total: 0)

Files potentially affected (total: 0)

📄 locales/da.json (total: 0)

Files potentially affected (total: 0)

📄 locales/de.json (total: 0)

Files potentially affected (total: 0)

📄 locales/en.json (total: 0)

Files potentially affected (total: 0)

📄 locales/es.json (total: 0)

Files potentially affected (total: 0)

📄 locales/fi.json (total: 0)

Files potentially affected (total: 0)

📄 locales/fr.json (total: 0)

Files potentially affected (total: 0)

📄 locales/hi.json (total: 0)

Files potentially affected (total: 0)

📄 locales/it.json (total: 0)

Files potentially affected (total: 0)

📄 locales/ja.json (total: 0)

Files potentially affected (total: 0)

📄 locales/ko.json (total: 0)

Files potentially affected (total: 0)

📄 locales/ms.json (total: 0)

Files potentially affected (total: 0)

📄 locales/nb.json (total: 0)

Files potentially affected (total: 0)

📄 locales/nl.json (total: 0)

Files potentially affected (total: 0)

📄 locales/pl.json (total: 0)

Files potentially affected (total: 0)

📄 locales/pt-BR.json (total: 0)

Files potentially affected (total: 0)

📄 locales/pt-PT.json (total: 0)

Files potentially affected (total: 0)

📄 locales/sv.json (total: 0)

Files potentially affected (total: 0)

📄 locales/th.json (total: 0)

Files potentially affected (total: 0)

📄 locales/tr.json (total: 0)

Files potentially affected (total: 0)

📄 locales/zh-CN.json (total: 0)

Files potentially affected (total: 0)

📄 locales/zh-TW.json (total: 0)

Files potentially affected (total: 0)

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

Files potentially affected (total: 53)

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

Files potentially affected (total: 52)

📄 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)

🧩 src/types.ts (total: 135)

Files potentially affected (total: 135)

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 · 2019-10-24T00:47:59Z

I dig this new prop!

ShaniqueS

Looks good to me 👍

kyledurand

Code and 🎩 look great. Thanks for taking this on!

kyledurand · 2019-11-18T20:32:54Z

src/components/Button/Button.scss

+
+.ConnectedDisclosureWrapper {
+  display: flex;
+  flex-wrap: nowrap;


nowrap is the default so you might not need to specify here.

kyledurand · 2019-11-19T20:05:59Z

src/components/Button/Button.tsx

+
+    const connectedDisclosureActivator = (
+      <button
+        type={type}


I think we might be safer by typing this as button vs passing the prop.

BPScott · 2019-11-19T21:02:34Z

@dleroux has some interesting stuff coming up that might allow us to do this split style with a ButtonGroup instead of a needing a new prop, you two should talk

chloerice · 2019-11-19T21:47:08Z

@dleroux has some interesting stuff coming up that might allow us to do this split style with a ButtonGroup instead of a needing a new prop, you two should talk

Thanks @BPScott! Dan reached out to me yesterday 👍 He's got some kinks to work out with the way the context works, but if it get's figured out and folks are on board with the asterisks then I'll end up closing this and opening a new PR adding a split button example to Button using the new ButtonGroup context.

dleroux · 2019-11-19T21:49:31Z

It’s now ready for review #2441

dleroux · 2019-11-21T13:28:07Z

It's ready for review again and much simpler #2441

simon-callsen · 2019-11-21T21:25:01Z

👋 Will this split action work when the button isn't Primary? We have a potential use case for this component when it's live that can be primary/not primary.

chloerice · 2019-12-05T04:38:32Z

👋 Will this split action work when the button isn't Primary? We have a potential use case for this component when it's live that can be primary/not primary.

Hey @simon-callsen 👋, yes the connectedDisclosure prop can be used with any button variant (except plain) 🎉

chloerice · 2019-12-05T23:43:32Z

src/styles/shared/_buttons.scss

@@ -201,6 +201,7 @@
  border-color: rgba($outline-color, 0.4);
  box-shadow: none;
  color: darken($outline-color, 20%);
+  @include recolor-icon(darken($outline-color, 20%));


Right now, a destructive outline icon button will have a bright red icon fill and darkened text. This isn't a variant represented in the style guide or in the UI kit, so I think this was just an oversight.

Before After

On the UI kit, the icon on the basic button is two tones lighter Ink lighter while the text is Ink, so I think the example on "Before" is actually correct.
Aso, the outline seems lighter than what's in the style guide.

Hmmm, those screenshots are from the style guide, @mirualves is your screenshot an outline button or a monochrome button?

My screenshot is from the outline monochrome button (style guide). I'm sorry for the late reply!

chloerice · 2019-12-05T23:48:02Z

src/styles/shared/_buttons.scss

@@ -247,6 +248,7 @@
      background: transparent;
      box-shadow: 0 0 0 border-width('base') var(--p-critical-border);
      color: var(--p-critical-link);
+      @include recolor-icon(var(--p-critical-link));


Again, parity between destructive outline button icon and text colors. cc @jessebc and @mirualves

Before After

Light mode

Dark mode

Yeah, the after makes sense to me. The contrast looks much better in both light and dark mode.

👀 Just a random question, but why are the outline and the text for the dark mode button two separate colors?

The short answer is for contrast ratios. But it is possible these colors will still be tweaked—at the moment it is more important the code is using the proper color role variable.

src/styles/shared/_buttons.scss

Update locales/hi.json Update locales/pl.json Update locales/pt-PT.json Update locales/zh-TW.json Update locales/tr.json Update locales/zh-CN.json Update locales/th.json Update locales/da.json Update locales/es.json Update locales/fr.json Update locales/nl.json Update locales/ko.json Update locales/ja.json Update locales/pt-BR.json Update locales/sv.json Update locales/ms.json Update locales/cs.json Update locales/fi.json Update locales/nb.json Update locales/it.json

chloerice force-pushed the button-connectedSegment branch 6 times, most recently from af9fce8 to 3d87c48 Compare October 23, 2019 22:28

chloerice force-pushed the button-connectedSegment branch 2 times, most recently from 35e9e22 to f332529 Compare November 15, 2019 18:37

chloerice marked this pull request as ready for review November 15, 2019 18:39

chloerice requested review from mirualves, ShaniqueS, kyledurand and dleroux November 15, 2019 18:39

chloerice force-pushed the button-connectedSegment branch from f332529 to 11fdf55 Compare November 15, 2019 20:00

ShaniqueS approved these changes Nov 18, 2019

View reviewed changes

kyledurand approved these changes Nov 19, 2019

View reviewed changes

dleroux mentioned this pull request Nov 26, 2019

[ButtonGroup] Add theme to ButtonGroup and move CSS to Button #2441

Merged

6 tasks

chloerice force-pushed the button-connectedSegment branch from 11fdf55 to 3939e9d Compare December 5, 2019 04:27

chloerice force-pushed the button-connectedSegment branch 3 times, most recently from 71f96b4 to 98b1c03 Compare December 5, 2019 23:04

chloerice requested a review from BPScott as a code owner December 5, 2019 23:04

chloerice requested review from danrosenthal and tmlayton as code owners December 5, 2019 23:04

chloerice force-pushed the button-connectedSegment branch from 98b1c03 to 3b48454 Compare December 5, 2019 23:14

chloerice requested review from jessebc and mirualves and removed request for mirualves December 5, 2019 23:30

chloerice force-pushed the button-connectedSegment branch 2 times, most recently from ff0b59f to 4cd4d7a Compare December 5, 2019 23:40

chloerice commented Dec 5, 2019

View reviewed changes

danrosenthal reviewed Dec 9, 2019

View reviewed changes

src/styles/shared/_buttons.scss Outdated Show resolved Hide resolved

chloerice force-pushed the button-connectedSegment branch from 4cd4d7a to bd4cabe Compare January 15, 2020 22:35

chloerice and others added 5 commits January 21, 2020 10:29

[Button] add support for split disclosure button

fd9caee

[Button] Minor content fixes to disclosure example

975830c

[Button] Adjust global theming styles for connected disclosure variant

fda5823

[UNRELEASED] Fix auto merged content

8e03336

chloerice force-pushed the button-connectedSegment branch from bd4cabe to 8e03336 Compare January 21, 2020 15:32

chloerice merged commit 7001057 into master Jan 21, 2020

chloerice deleted the button-connectedSegment branch January 21, 2020 16:10

dleroux temporarily deployed to production January 27, 2020 13:38 Inactive

chloerice mentioned this pull request Feb 7, 2020

Extend support for split button to ContextualSaveBar and Page #2731

Closed

6 tasks

[Button] add support for split disclosure button #2329

[Button] add support for split disclosure button #2329

Conversation

chloerice commented Oct 18, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

To tophat in Chroma deployment

To tophat locally

🎩 checklist

github-actions bot commented Oct 23, 2019 • edited Loading

Details

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 53)

Files potentially affected (total: 52)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 135)

tmlayton commented Oct 24, 2019

ShaniqueS left a comment

Choose a reason for hiding this comment

kyledurand left a comment

Choose a reason for hiding this comment

kyledurand Nov 18, 2019

Choose a reason for hiding this comment

kyledurand Nov 19, 2019

Choose a reason for hiding this comment

chloerice Nov 19, 2019

Choose a reason for hiding this comment

BPScott commented Nov 19, 2019

chloerice commented Nov 19, 2019

dleroux commented Nov 19, 2019

dleroux commented Nov 21, 2019

simon-callsen commented Nov 21, 2019

chloerice commented Dec 5, 2019 • edited Loading

chloerice Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

mirualves Dec 6, 2019

Choose a reason for hiding this comment

chloerice Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

mirualves Dec 18, 2019

Choose a reason for hiding this comment

chloerice Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

mirualves Dec 6, 2019

Choose a reason for hiding this comment

ShaniqueS Dec 6, 2019

Choose a reason for hiding this comment

tmlayton Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

chloerice commented Oct 18, 2019 •

edited

Loading

github-actions bot commented Oct 23, 2019 •

edited

Loading

chloerice commented Dec 5, 2019 •

edited

Loading

chloerice Dec 5, 2019 •

edited

Loading

chloerice Dec 6, 2019 •

edited

Loading

chloerice Dec 5, 2019 •

edited

Loading

tmlayton Dec 9, 2019 •

edited

Loading