Skip to content

Conversation

tmlayton
Copy link
Contributor

WHY are these changes introduced?

Porting over from https://github.com/Shopify/polaris-react-deprecated/pull/1820

WHAT is this pull request doing?

Add events for the aforementioned.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {Page, Button} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page title="Playground">
        <Button onKeyPress={log} onKeyDown={log} onKeyUp={log}>
          Test
        </Button>
      </Page>
    );
  }
}

function log(event: KeyboardEvent) {
  console.log(event.keyCode);
}

🎩 checklist

@BPScott BPScott requested a deployment to polaris-react-pr-860 January 11, 2019 20:52 Abandoned
@tmlayton tmlayton force-pushed the button-keyboard-events branch from 9861600 to 3392409 Compare January 11, 2019 21:58
@BPScott BPScott temporarily deployed to polaris-react-pr-860 January 11, 2019 21:58 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-860 January 11, 2019 22:05 Inactive
Copy link

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

Code and tests LGTM!

Copy link

@dpersing dpersing left a comment

Choose a reason for hiding this comment

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

I think we will want to provide some README.md/style guide documentation around giving merchants instructions if non-standard button events are used. I can draft something as part of the accessibility guidelines for buttons (current draft here), but I think it might warrant expanding that into an section on non-standard interactions on the Interaction states page in the style guide.

@danrosenthal @francinen What would you think about that type of guidance, and/or perhaps working on a pattern guide around events? We could use the drag-and-drop interaction as an example.

I think it would also be worthwhile to consider making a drag-and-drop component.

cc @tmlayton

@francinen
Copy link
Contributor

@dpersing I think that would be worth investing in. Is there a specific approach to providing that context to merchants that the documentation should endorse, e.g., using aria-describedby to point to a node containing text instructions for interacting with the component?

@dpersing
Copy link

using aria-describedby to point to a node containing text instructions for interacting with the component

@francinen I think having visible instructions (for sighted keyboard users) tied to the feature with aria-describedby or similar would be a great example. To conserve space, maybe the instructions are in a tooltip that comes before the drag-and-drop controls?

@tmlayton
Copy link
Contributor Author

I think we will want to provide some README.md/style guide documentation around giving merchants instructions if non-standard button events are used

@dpersing do you think we should make it part of this PR?

@dpersing
Copy link

@tmlayton I think it would make sense to include in component-level accessibility documentation, which is dependent on https://github.com/Shopify/polaris-styleguide/pull/2493. I think shipping before that's complete would be totally fine. I've included a call-out for non-standard button events in what will get published when we can.

Copy link
Contributor

@francinen francinen left a comment

Choose a reason for hiding this comment

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

Thank you!

@tmlayton tmlayton force-pushed the button-keyboard-events branch from 65d3b10 to ee27316 Compare January 16, 2019 22:12
@tmlayton tmlayton merged commit dbd6173 into master Jan 16, 2019
@tmlayton tmlayton deleted the button-keyboard-events branch January 16, 2019 23:04
@BPScott BPScott temporarily deployed to production January 28, 2019 16:33 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants