[BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList #2467

dleroux · 2019-11-25T20:48:48Z

WHY are these changes introduced?

partly addresses: https://github.com/Shopify/polaris-ux/issues/360 and https://github.com/Shopify/polaris-ux/issues/361

While applying the design language to ButtonGroup, we were able to change how ButtonGroups work. This allowed us to refactor the ResourceList BulkActions and remove the custom components.

WHAT is this pull request doing?

applies design language
makes use of Buttongroups
removes the need for custom BulkAction buttons

How to 🎩

To 🎩 ensure BulkActions are working as expected in Storybook. Including small screen, and select mode.

Also use yarn build-consumer web

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

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

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </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-25T20:50:55Z

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

Files modified	12
Files potentially affected	4

Details

All files potentially affected (total: 4)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/ResourceItem/ResourceItem.scss (total: 2)

Files potentially affected (total: 2)

🧩 src/components/ResourceList/ResourceList.tsx (total: 0)

Files potentially affected (total: 0)

🎨 src/components/ResourceList/components/BulkActions/BulkActions.scss (total: 2)

Files potentially affected (total: 2)

🧩 src/components/ResourceList/components/BulkActions/BulkActions.tsx (total: 1)

Files potentially affected (total: 1)

/src/components/ResourceList/ResourceList.tsx

🧩 src/components/ResourceList/components/BulkActions/components/BulkActionButton/BulkActionButton.tsx (total: 2)

Files potentially affected (total: 2)

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

Files potentially affected (total: 0)

🎨 src/components/ResourceList/components/CheckableButton/CheckableButton.scss (total: 3)

Files potentially affected (total: 3)

🧩 src/components/ResourceList/components/CheckableButton/CheckableButton.tsx (total: 2)

Files potentially affected (total: 2)

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

Files potentially affected (total: 0)

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

Files potentially affected (total: 0)

📄 tests/build.test.js (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.

dleroux · 2019-11-26T18:18:04Z

src/components/ResourceList/components/BulkActions/BulkActions.scss

+
+  // We need the first item of button group on small screen to fill the space
+  // stylelint-disable selector-max-specificity, selector-max-class
+  @include breakpoint-before(resource-list(breakpoint-small)) {


Our fullWidth makes every button equal width. On a small screen, bulk actions require only the checkable button to extend the full width.

We toyed with the idea of adding a prop to ButtonGroup which was confusing and this being the only use case where we have seen this we decided to move forward with this approach. We added a test to ensure that if ever there's mark-up change, this will be flagged.

This is the trade-off of being able the use ButtonGroup and allowing us to remove quite a bit of custom components.

dleroux · 2019-11-26T18:38:53Z

fixing failing tests.

dleroux · 2019-11-26T20:15:51Z

The code cov is complaining about unstableGlobalTheming && styles.globalTheming, Are we ignoring these? @tmlayton

tmlayton · 2019-11-27T01:58:38Z

We have been adding tests if using the features hook

danrosenthal · 2019-11-27T14:01:23Z

src/components/ResourceList/components/BulkActions/BulkActions.scss

@@ -6,23 +6,6 @@ $bulk-actions-button-stacking-order: (
 );
 $bulk-actions-offset-slide-in-start: rem(-40px);

-.Button {
-  @include text-style-button;
-  @include button-base;


danrosenthal · 2019-11-27T14:01:43Z

src/components/ResourceList/components/BulkActions/BulkActions.scss

-    &:focus {
-      z-index: 1;
-    }
-  }


danrosenthal · 2019-11-27T14:01:54Z

src/components/ResourceList/components/BulkActions/BulkActions.scss

-    @include recolor-icon(color('ink', 'lightest'));
-  }
-}
-


danrosenthal · 2019-11-27T14:05:44Z

Your ButtonGroup modifications really unlocked some powerful changes here. Nice work.

Noticing that the checkable button icon is colored as a normal icon. That makes sense since we don't have indigo in the pallet any longer, but it makes it invisible in dark mode since the CheckBox component isn't converted to use the color system yet. It might be worth just leaving the icon with the non-global-theming styles (leaving it indigo) until we convert Checkbox, so things don't appear broken.

dleroux · 2019-11-27T14:34:46Z

Yes it did unlock quite a bit. It's also allowing to remove ToggleButton in web and all the custom stuff associated with the RTE toolbar.

You are right, the checkbox will get branded automatically when we apply the branding.

This is ready for review 🙏

danrosenthal

Nice work!

dleroux changed the base branch from master to buttongroup-context November 25, 2019 20:49

dleroux force-pushed the buttongroup-context branch 2 times, most recently from 337cccd to 12714f4 Compare November 26, 2019 12:52

dleroux force-pushed the cs-bulkactions branch 2 times, most recently from 1085809 to 7aec9fb Compare November 26, 2019 18:10

dleroux changed the base branch from buttongroup-context to master November 26, 2019 18:11

dleroux commented Nov 26, 2019

View reviewed changes

dleroux force-pushed the cs-bulkactions branch 2 times, most recently from 34ca7a1 to d5085d9 Compare November 26, 2019 18:21

dleroux changed the title ~~[WIP] Add theming to BulkAction and remove custom ButtonGroup from ResourceList~~ [WIP][BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList Nov 26, 2019

dleroux requested review from chloerice, tmlayton and danrosenthal November 26, 2019 18:35

dleroux changed the title ~~[WIP][BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList~~ [BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList Nov 26, 2019

dleroux changed the title ~~[BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList~~ [WIP][BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList Nov 26, 2019

dleroux force-pushed the cs-bulkactions branch from d5085d9 to 27fa0e6 Compare November 26, 2019 19:09

danrosenthal reviewed Nov 27, 2019

View reviewed changes

src/components/ResourceList/components/BulkActions/BulkActions.scss

&:focus {

z-index: 1;

}

}

Copy link

Contributor

danrosenthal Nov 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

danrosenthal reviewed Nov 27, 2019

View reviewed changes

dleroux force-pushed the cs-bulkactions branch from f19301c to 415610e Compare November 27, 2019 14:09

update bulkactions

dcf878b

dleroux force-pushed the cs-bulkactions branch from 415610e to dcf878b Compare November 27, 2019 14:25

dleroux requested a review from danrosenthal November 27, 2019 14:31

dleroux changed the title ~~[WIP][BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList~~ [BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList Nov 27, 2019

danrosenthal approved these changes Nov 27, 2019

View reviewed changes

dleroux merged commit 7036601 into master Nov 27, 2019

dleroux deleted the cs-bulkactions branch November 27, 2019 15:35

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

dleroux mentioned this pull request Dec 5, 2019

[CheckableButton] add border-right when there are no actions #2516

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList #2467

[BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList #2467

dleroux commented Nov 25, 2019 •

edited

Loading

github-actions bot commented Nov 25, 2019 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 2)

Files potentially affected (total: 0)

Files potentially affected (total: 2)

Files potentially affected (total: 1)

Files potentially affected (total: 2)

Files potentially affected (total: 0)

Files potentially affected (total: 3)

Files potentially affected (total: 2)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

dleroux Nov 26, 2019

dleroux commented Nov 26, 2019

dleroux commented Nov 26, 2019

tmlayton commented Nov 27, 2019 •

edited

Loading

danrosenthal Nov 27, 2019

danrosenthal Nov 27, 2019

danrosenthal Nov 27, 2019

danrosenthal commented Nov 27, 2019 •

edited

Loading

dleroux commented Nov 27, 2019 •

edited

Loading

danrosenthal left a comment

[BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList #2467

[BulkActions] Add theming to BulkAction and remove custom ButtonGroup from ResourceList #2467

Conversation

dleroux commented Nov 25, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

github-actions bot commented Nov 25, 2019 • edited Loading

Details

Files potentially affected (total: 0)

Files potentially affected (total: 2)

Files potentially affected (total: 0)

Files potentially affected (total: 2)

Files potentially affected (total: 1)

Files potentially affected (total: 2)

Files potentially affected (total: 0)

Files potentially affected (total: 3)

Files potentially affected (total: 2)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

dleroux Nov 26, 2019

Choose a reason for hiding this comment

dleroux commented Nov 26, 2019

dleroux commented Nov 26, 2019

tmlayton commented Nov 27, 2019 • edited Loading

danrosenthal Nov 27, 2019

Choose a reason for hiding this comment

danrosenthal Nov 27, 2019

Choose a reason for hiding this comment

danrosenthal Nov 27, 2019

Choose a reason for hiding this comment

danrosenthal commented Nov 27, 2019 • edited Loading

dleroux commented Nov 27, 2019 • edited Loading

danrosenthal left a comment

Choose a reason for hiding this comment

dleroux commented Nov 25, 2019 •

edited

Loading

github-actions bot commented Nov 25, 2019 •

edited

Loading

tmlayton commented Nov 27, 2019 •

edited

Loading

danrosenthal commented Nov 27, 2019 •

edited

Loading

dleroux commented Nov 27, 2019 •

edited

Loading