[ResourceList-BulkAction][Checkbox] Handle focus on bulk actions and converts checkbox into a functional component. #2138

dleroux · 2019-09-13T14:27:11Z

WHY are these changes introduced?

Fixes #792

WHAT is this pull request doing?

I believe this is the last of the ResourceList a11y issues outline in [#792].

In order to switch focus between the plain CheckableButton checkbox and the ones inside the BulkActions, we needed to keep track of the button that was clicked and which of the 3 possible buttons that would receive focus.

The 3 buttons are tracked via context.
handleToggleAll is only called when the bulk action or plain button are clicked. From there, depending on screen size we focus the right button.
To enable focus on the checkbox I needed to forwardRef on the checkbox. According to this: https://reactjs.org/docs/forwarding-refs.html#note-for-component-library-maintainers This would be a breaking changes. Discussed with @AndrewMusgrave and we're not sure we agree in our case. @BPScott thoughts?

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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

import React from 'react';
import {
  Page,
  ResourceList,
  Card,
  Avatar,
  ResourceItem,
  TextStyle,
} from '../src';

export function Playground() {
  return (
    <Page>
      <ResourceListExample />
    </Page>
  );
}
class ResourceListExample extends React.Component {
  state = {
    selectedItems: [],
  };

  handleSelectionChange = (selectedItems) => {
    this.setState({selectedItems});
  };

  renderItem = (item) => {
    const {id, url, name, location} = item;
    const media = <Avatar customer size="medium" name={name} />;
    return (
      <ResourceItem
        id={id}
        url={url}
        media={media}
        accessibilityLabel={`View details for ${name}`}
      >
        <h3>
          <TextStyle variation="strong">{name}</TextStyle>
        </h3>
        <div>{location}</div>
      </ResourceItem>
    );
  };

  render() {
    const resourceName = {
      singular: 'customer',
      plural: 'customers',
    };
    const items = [
      {
        id: 341,
        url: 'customers/341',
        name: 'Mae Jemison',
        location: 'Decatur, USA',
      },
      {
        id: 256,
        url: 'customers/256',
        name: 'Ellen Ochoa',
        location: 'Los Angeles, USA',
      },
    ];
    const promotedBulkActions = [
      {
        content: 'Edit customers',
        onAction: () => console.log('Todo: implement bulk edit'),
      },
    ];
    const bulkActions = [
      {
        content: 'Add tags',
        onAction: () => console.log('Todo: implement bulk add tags'),
      },
      {
        content: 'Remove tags',
        onAction: () => console.log('Todo: implement bulk remove tags'),
      },
      {
        content: 'Delete customers',
        onAction: () => console.log('Todo: implement bulk delete'),
      },
    ];
    return (
      <Card>
        <ResourceList
          resourceName={resourceName}
          items={items}
          renderItem={this.renderItem}
          selectedItems={this.state.selectedItems}
          onSelectionChange={this.handleSelectionChange}
          promotedBulkActions={promotedBulkActions}
          bulkActions={bulkActions}
        />
      </Card>
    );
  }
}

🎩 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

BPScott · 2019-09-13T17:49:46Z

src/components/Checkbox/Checkbox.tsx

-    const {onChange, id, disabled} = this.props;
-
-    if (onChange == null || this.inputNode.current == null || disabled) {
+const ImperitaveCheckbox: RefForwardingComponent<


I think we'd be able to sidestep this explicit typing stuff if we define this inline rather than a separate function

export const Checkbox = React.forwardRef(function Checkbox({ /* Props here */ }) { /* Blah */ });

I couldn't get this to work for some reason. I'll give it another go.

BPScott · 2019-09-13T17:50:55Z

src/components/Checkbox/Checkbox.tsx

+  const inputNode = useRef<HTMLInputElement>(null);
+
+  const generatedId = useRef(getUniqueID());
+  const id = idProp || generatedId.current;


use useUniqueId instead of createUniqueIDFactory and ref dancing

dleroux · 2019-09-16T13:45:59Z

After a discussion wit @AndrewMusgrave and @BPScott regarding this being a breaking change we agreed that in our case we wouldn't consider this a breaking change because even though the type for Checkbox is changing, we did not expose any public API or forwardRef on Checkbox, therefore, this shouldn't affect anything we support today.

BPScott · 2019-09-16T18:07:01Z

src/components/Checkbox/Checkbox.tsx

+      if (onChange == null || inputNode.current == null || disabled) {
+        return;
+      }
+      onChange(!inputNode.current.checked, id as any);


It looks like id is now typed to be string so do you need that as any?

AndrewMusgrave

Looks like we might be able to infer the imperative handler type with one of these

AndrewMusgrave · 2019-09-17T16:08:07Z

src/components/ResourceList/components/BulkActions/BulkActions.tsx

@@ -27,6 +27,8 @@ const MAX_PROMOTED_ACTIONS = 2;
 export interface BulkActionsProps {
  /** Visually hidden text for screen readers */
  accessibilityLabel?: string;
+  /** Whether to render the small screen BulkActions or not */
+  smallScreen?: boolean;


Rather than adding this to the public API - should we use #2117 ?

In this case smallScreen is const random number for the resourceList. I could duplicate the code or add it to context, but I opted for a prop. What do you think?

AndrewMusgrave · 2019-09-17T16:08:37Z

src/components/ResourceList/components/BulkActions/BulkActions.tsx

@@ -363,11 +366,12 @@ class BulkActions extends React.PureComponent<CombinedProps, State> {
      disabled,
    };

-    const smallScreenGroup = (
+    const smallScreenGroup = smallScreen ? (


Love how we're only rendering these when we need to 😍

AndrewMusgrave · 2019-09-17T16:11:18Z

src/components/ResourceList/components/BulkActions/tests/BulkActions.test.tsx

+        const bulkActions = mountWithAppProvider(
+          <BulkActions {...bulkActionProps} selectMode />,
+        );
+        const smallGroup = findByTestID(bulkActions, 'smallGroup');


An alternative to testID's could be find(Transition) since you were worried about using them

I wanted to use findWhere and use the key, but the keys get striped. Just using Find doesn't guarantee us which will be found.

AndrewMusgrave · 2019-09-17T16:12:25Z

src/components/ResourceList/components/BulkActions/tests/BulkActions.test.tsx

+        const largeGroup = findByTestID(bulkActions, 'largeGroup');
+
+        expect(smallGroup.exists()).toBe(false);
+        expect(largeGroup.exists()).toBe(true);


expect(bulkActions ).toContainReactComponentTimes(Transition, 1);

Same as above. No guarantee which was found.

AndrewMusgrave · 2019-09-17T18:19:46Z

src/components/ResourceList/tests/ResourceList.test.tsx

+            'input[type="checkbox"]',
+          );
+
+          expect(checkBox.instance()).toBe(document.activeElement);


instance returns a react component, however, getDOMNode returns the dom element

Interesting how this did pass though.

AndrewMusgrave · 2019-09-17T18:20:16Z

src/components/ResourceList/tests/ResourceList.test.tsx

+            />,
+          );
+
+          const deselectAllCheckableButton = resourceList.findWhere(


With react-testing you can automatically filter by props in the matcher

AndrewMusgrave · 2019-09-17T18:29:49Z

src/components/ResourceList/ResourceList.tsx

+    button: CheckableButtonNode,
+  ) => {
+    const {checkableButtons} = this.state;
+    checkableButtons.set(key, button);


Uh-oh we're mutating state here 🚫

BPScott

Conversion of Checkbox looks good. I don't know enough about ResourceList to be able to give a decent review on that.

One small note inline about note about not making CheckboxHandles public but other than that I'm happy

BPScott · 2019-09-18T00:03:32Z

src/components/index.ts

@@ -28,7 +28,7 @@ export {Caption, CaptionProps} from './Caption';

 export {Card, CardProps} from './Card';

-export {Checkbox, CheckboxProps} from './Checkbox';
+export {Checkbox, CheckboxProps, CheckboxHandles} from './Checkbox';


I'd like to avoid exporting this publicly, people shouldn't need it

fair enough. I had to disable strict-component-boundaries in the types file to do this.

dpersing · 2019-09-23T21:23:01Z

👋 @BPScott and @AndrewMusgrave, any chance you've taken a look at this yet?

AndrewMusgrave

Using the keyboard it looks like the correct checkbox has focus however it doesn't show visually (https://cl.ly/12e433c1102a). I'm using spacebar to toggle. Left a few comments with thoughts/questions, no strong feelings so do with it if you please 😄 Awesome job! This looks amazing 🎉

AndrewMusgrave · 2019-09-24T17:14:05Z

src/components/ResourceList/tests/ResourceList.test.tsx

+          );
+
+          const selectAllCheckableButton = resourceList.findWhere(
+            (wrap) => wrap.is(CheckableButton) && wrap.prop('plain') === true,


No strong feelings, we usually coerce these types as values rather than compare against a boolean but I'll leave the choice up to you

Suggested change

(wrap) => wrap.is(CheckableButton) && wrap.prop('plain') === true,

(wrap) => wrap.is(CheckableButton) && wrap.prop('plain'),

AndrewMusgrave · 2019-09-24T17:16:14Z

src/components/ResourceList/tests/ResourceList.test.tsx

+            .findWhere(
+              (wrap) => wrap.is(CheckableButton) && !wrap.prop('plain'),
+            )
+            .find('input[type="checkbox"]');


Just curious why you didn't include this in findWhere or before the findWhere (both should reduce the number of operations, although probably by a negligible amount)(not asking for a code change, just curious)

🤔 the findWhere will return the wrap based on the filtering condition. I'm after the checkbox inside the wrap. Putting the find before would not allow me to findWhere based on parents asaik. In any case, this has been simplified, because as you suggested I made the findWhere into a function.

AndrewMusgrave · 2019-09-24T17:17:46Z

src/components/ResourceList/tests/ResourceList.test.tsx

+            />,
+          );
+
+          const deselectAllCheckableButton = resourceList.findWhere(


Seems like we use this logic a lot, might be worth bringing it out into a function

function deselectAllCheckableButtons(wrapper) {} | function selectedAllCheckableButtons(wrapper)

AndrewMusgrave · 2019-09-24T17:18:13Z

src/components/ResourceList/tests/ResourceList.test.tsx

+
+          const selectAllCheckableCheckbox = resourceList
+            .findWhere(
+              (wrap) => wrap.is(CheckableButton) && wrap.prop('plain') === true,


Same comment as a few . above

Suggested change

(wrap) => wrap.is(CheckableButton) && wrap.prop('plain') === true,

(wrap) => wrap.is(CheckableButton) && wrap.prop('plain'),

AndrewMusgrave · 2019-09-24T17:23:41Z

src/components/ResourceList/components/CheckableButton/CheckableButton.tsx

@@ -9,6 +14,7 @@ export interface CheckableButtonProps {
  label?: string;
  selected?: boolean | 'indeterminate';
  selectMode?: boolean;
+  smallScreen?: boolean;


Should we create an issue to add RL smallScreen to media query provider and remove this prop?

If you feel like we should we could do that. I actually don't mind the prop here making this a controllable dumb component.

AndrewMusgrave · 2019-09-24T17:27:30Z

src/components/ResourceList/components/CheckableButton/CheckableButton.tsx

+
+  let currentKey: CheckableButtonKey = 'plain';
+
+  if (plain) {


Curious what you think about a switch statement here?

case plain: break; case smallScreen: currentKey = 'bulkSm'; break; default: currentKey = 'bulkLg';

or if there's a condition that lets us remove currentKey = 'plain'; or default currentKey to undefined. Just some written thoughts, don't let this block you from 🐑 🇮🇹

in this case, we're switching on 2 different props. I can simply by defaulting to the else instead.

AndrewMusgrave · 2019-09-24T17:28:30Z

src/utilities/resource-list/types.ts

 export type ResourceListSelectedItems = string[] | 'All';

+export type CheckableButtonNode = CheckboxHandles;


Do we need this type?

Not really I guess. I wasn't thinking it made sense just because of the naming, but I can just us the CheckboxHandles. I'll remove it since it brings up the question.

dleroux · 2019-09-25T18:59:26Z

Using the keyboard it looks like the correct checkbox has focus however it doesn't show visually

Not sure why this is, everything checks out and the input does get focus. For some reason with the keyboard it doesn't trigger the visual change on the checkbox. Tried forcing a reflow and it doesn't work either. Sadly the only thing that does work is requestAnimationFrame, so I had to mock it.

The rest of the suggestions have been applied.

dpersing · 2019-09-30T16:10:23Z

or some reason with the keyboard it doesn't trigger the visual change on the checkbox.

.Checkbox-Input_1uEY6 {
    position: absolute !important;
    top: 0;
    clip: rect(1px, 1px, 1px, 1px) !important;
    overflow: hidden !important;
    height: 1px !important;
    width: 1px !important;
    padding: 0 !important;
    border: 0 !important;
}

@dleroux I suspect this is because the actual checkbox is visually hidden, and the visually mocked on is getting updated when the user unchecks it. Focus is on the "old" version of the visual checkbox.

If we can avoid hiding the actual checkbox visually, this issue should go away.

AndrewMusgrave · 2019-09-30T20:08:53Z

Is this ready for another round of reviews?

dleroux · 2019-10-02T17:12:02Z

Yes it is.

AndrewMusgrave · 2019-10-03T18:06:22Z

src/components/ResourceList/ResourceList.tsx

    if (onSelectionChange) {
      onSelectionChange(newlySelectedItems);
    }
+
+    // setTimeout ensures execution after the Transition on BulkActions
+    setTimeout(() => {


cc/ @beefchimi similar to what we were looking at yesterday

Similar, except this is able to get away with a 0ms Timeout 😄

In this case, is requestAnimationFrame an alternative solution the problem? Also... is this needed specifically because of being nested in react-transition-group?

Yes, it is needed because of react-transition-group. They trigger their transition using setTimeout. So using setTimeout here ensures that theirs is called first. requestAnimationFrame was working, but because they are using setTimeout we felt it was better here.

AndrewMusgrave

🎩 looked ✅

github-actions · 2019-10-03T19:07:48Z

Results

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

Files modified	13
Files potentially affected	135

Details

All files potentially affected (total: 135)

❔ UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Checkbox/Checkbox.tsx (total: 5)

Files potentially affected (total: 5)

🧩 src/components/Checkbox/index.ts (total: 5)

Files potentially affected (total: 5)

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

Files potentially affected (total: 0)

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

Files potentially affected (total: 0)

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

Files potentially affected (total: 1)

/src/components/ResourceList/ResourceList.tsx

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

Files potentially affected (total: 0)

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

Files potentially affected (total: 2)

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

Files potentially affected (total: 0)

🧩 src/types.ts (total: 135)

Files potentially affected (total: 135)

🧩 src/utilities/resource-list/context.ts (total: 7)

Files potentially affected (total: 7)

🧩 src/utilities/resource-list/index.ts (total: 6)

Files potentially affected (total: 6)

🧩 src/utilities/resource-list/types.ts (total: 8)

Files potentially affected (total: 8)

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.

BPScott mentioned this pull request Sep 13, 2019

Post v4 cleanup work #1995

Closed

BPScott reviewed Sep 13, 2019

View reviewed changes

dleroux force-pushed the 792-resource-list-focus branch 9 times, most recently from 52e20ab to 693b755 Compare September 16, 2019 13:41

dleroux changed the title ~~[WIP][ResourceList-BulkAction][Checkbox] Handle focus on bulk actions and converts checkbox into a functional component.~~ [ResourceList-BulkAction][Checkbox] Handle focus on bulk actions and converts checkbox into a functional component. Sep 16, 2019

dleroux requested review from AndrewMusgrave and BPScott September 16, 2019 13:49

dleroux force-pushed the 792-resource-list-focus branch from 693b755 to e326ded Compare September 16, 2019 13:54

BPScott reviewed Sep 16, 2019

View reviewed changes

dleroux force-pushed the 792-resource-list-focus branch from e326ded to a387db2 Compare September 17, 2019 14:33

dleroux requested a review from BPScott September 17, 2019 14:33

AndrewMusgrave reviewed Sep 17, 2019

View reviewed changes

dleroux force-pushed the 792-resource-list-focus branch 2 times, most recently from 15d8a4d to 33059ae Compare September 17, 2019 22:58

BPScott reviewed Sep 18, 2019

View reviewed changes

dleroux force-pushed the 792-resource-list-focus branch 3 times, most recently from c8a6364 to 7d79ac3 Compare September 18, 2019 12:37

dleroux requested a review from BPScott September 18, 2019 18:08

dleroux force-pushed the 792-resource-list-focus branch from 7d79ac3 to 6c6ac69 Compare September 18, 2019 18:08

dleroux requested a review from AndrewMusgrave September 18, 2019 18:08

dleroux force-pushed the 792-resource-list-focus branch 2 times, most recently from 4fe585d to dcdb251 Compare September 24, 2019 13:03

AndrewMusgrave reviewed Sep 24, 2019

View reviewed changes

dleroux force-pushed the 792-resource-list-focus branch from dcdb251 to 78cd5f2 Compare September 25, 2019 18:56

dleroux requested a review from AndrewMusgrave September 26, 2019 13:26

dleroux force-pushed the 792-resource-list-focus branch 2 times, most recently from 0aba4d3 to d5636b3 Compare September 30, 2019 15:36

AndrewMusgrave reviewed Oct 3, 2019

View reviewed changes

AndrewMusgrave approved these changes Oct 3, 2019

View reviewed changes

fixed focus on bulk action and convert checkbox to functional component

a65d5d3

dleroux force-pushed the 792-resource-list-focus branch from d5636b3 to a65d5d3 Compare October 3, 2019 19:05

dleroux merged commit f323ec9 into master Oct 4, 2019

dleroux deleted the 792-resource-list-focus branch October 4, 2019 12:33

dleroux temporarily deployed to production October 8, 2019 11:46 Inactive

	(wrap) => wrap.is(CheckableButton) && wrap.prop('plain') === true,
	(wrap) => wrap.is(CheckableButton) && wrap.prop('plain'),

		export type ResourceListSelectedItems = string[] \| 'All';

		export type CheckableButtonNode = CheckboxHandles;

[ResourceList-BulkAction][Checkbox] Handle focus on bulk actions and converts checkbox into a functional component. #2138

[ResourceList-BulkAction][Checkbox] Handle focus on bulk actions and converts checkbox into a functional component. #2138

Conversation

dleroux commented Sep 13, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dleroux commented Sep 16, 2019

Choose a reason for hiding this comment

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BPScott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dleroux Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

dpersing commented Sep 23, 2019

AndrewMusgrave left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dleroux commented Sep 25, 2019

dpersing commented Sep 30, 2019

AndrewMusgrave commented Sep 30, 2019 • edited Loading

dleroux commented Oct 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

Results

Details

Files potentially affected (total: 0)

Files potentially affected (total: 5)

Files potentially affected (total: 5)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Files potentially affected (total: 0)

Files potentially affected (total: 2)

Files potentially affected (total: 0)

Files potentially affected (total: 135)

Files potentially affected (total: 7)

Files potentially affected (total: 6)

Files potentially affected (total: 8)

dleroux commented Sep 13, 2019 •

edited

Loading

dleroux Sep 18, 2019 •

edited

Loading

AndrewMusgrave left a comment •

edited

Loading

AndrewMusgrave commented Sep 30, 2019 •

edited

Loading

github-actions bot commented Oct 3, 2019 •

edited

Loading