Revising drag and drop for accessible interactive and complex patterns. by 1Copenut · Pull Request #5568 · elastic/eui

1Copenut · 2022-01-26T22:58:51Z

Summary

Our interactive elements drag and drop pattern had focusable buttons that could not be reached by keyboard, and wouldn't announce to screen readers. We also had a draggable group that was a div[role="button"] which was preventing keyboard navigation. I refactored both patterns to retain the existing visual design as much as possible, and add the needed updates to pass an axe check and improve keyboard and screen reader experience.

Closes #5294

Design change

I made a visual change to the interactive elements pattern by leveraging the custom drag handle. I went this direction to fix a nested focusable element error. If the container will receive keyboard focus, the interactive element(s) will not be usable by keyboard events, and will not be announced to screen readers. I am attaching a screenshot of my proposed change below, and happy to make changes to visual design as needed.

Checklist

kibanamachine · 2022-01-26T23:32:21Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5568/

* Adding documentation for custom and interactive patterns props. * Adding changelog entry.

src-docs/src/views/drag_and_drop/drag_and_drop_complex.js

src-docs/src/views/drag_and_drop/drag_and_drop_disable_blocking.js

src/components/drag_and_drop/draggable.tsx

kibanamachine · 2022-01-27T16:41:54Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5568/

src/components/drag_and_drop/_draggable.scss

src/components/drag_and_drop/draggable.tsx

cee-chen · 2022-01-27T17:29:40Z

QA looks great to me, and I personally strongly prefer the cognitive separation of the drag handle vs. the buttons on the "Interactive elements" demo 👏

Code generally looks good - my blocking change requests are around ensuring we add unit tests for the new props, and not adding demo-specific CSS to our source code.

thompsongl · 2022-01-27T20:58:53Z

personally strongly prefer the cognitive separation of the drag handle vs. the buttons on the "Interactive elements" demo 👏

Agreed!

kibanamachine · 2022-01-27T22:20:18Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5568/

src/components/drag_and_drop/draggable.test.tsx

src/components/drag_and_drop/draggable.tsx

cee-chen · 2022-01-27T23:08:28Z

src-docs/src/views/drag_and_drop/drag_and_drop_disable_blocking.js

+              <EuiPanel className="custom" paddingSize="m">
+                <EuiFlexGroup alignItems="center" gutterSize="l">


Super visual minor spacing suggestions, I think this brings it back closer to what it looked like originally. Feel free to wait for @miukimiu on this one though

Suggested change

<EuiPanel className="custom" paddingSize="m">

<EuiFlexGroup alignItems="center" gutterSize="l">

<EuiPanel className="custom" paddingSize="s">

<EuiFlexGroup alignItems="center" gutterSize="m">

I added a new screenshot in the description to highlight this change.

kibanamachine · 2022-01-27T23:17:45Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5568/

kibanamachine · 2022-01-28T14:58:03Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5568/

cee-chen

Woohoo, thanks for the responsiveness and feedback rounds! This looks awesome. @miukimiu, did you want to take a look at this before merging?

src/components/drag_and_drop/draggable.tsx

elizabetdev

Thanks, @1Copenut! The PR is looking good! 🎉

I tested in Chrome, Safari, Firefox, and it works well! 👍🏽

I have a few suggestions below.

Let me know if there's any problem on replacing the <div {...provided.dragHandleProps} /> with a <EuiPanel {...provided.dragHandleProps} />. If not, I think visually it would look better.

If we go for this suggested change I would also consider changing the Custom drag handle example to use the <EuiPanel /> instead of a <div />. So both examples, Custom drag handle and Interactive elements would have similar elements.

elizabetdev · 2022-01-28T18:38:23Z

src-docs/src/views/drag_and_drop/drag_and_drop_disable_blocking.js

+                <EuiFlexGroup alignItems="center" gutterSize="m">
+                  <EuiFlexItem grow={false}>
+                    <div {...provided.dragHandleProps} aria-label="Drag Handle">
+                      <EuiIcon type="grab" />
+                    </div>


I would suggest using a transparent EuiPanel with a padding instead of a div.

This would give us a bigger size draggable area.

It would ensure more consistent spacing between the icon and the button.

Suggested change

<EuiFlexGroup alignItems="center" gutterSize="m">

<EuiFlexItem grow={false}>

<div {...provided.dragHandleProps} aria-label="Drag Handle">

<EuiIcon type="grab" />

</div>

<EuiFlexGroup alignItems="center" gutterSize="s">

<EuiFlexItem grow={false}>

<EuiPanel

color="transparent"

paddingSize="s"

{...provided.dragHandleProps}

aria-label="Drag Handle"

>

<EuiIcon type="grab" />

</EuiPanel>

Thank you @miukimiu! These suggestions were easy to implement, and made the button hit space larger and more consistent. Ready for next review.

elizabetdev · 2022-01-28T18:39:01Z

src-docs/src/views/drag_and_drop/drag_and_drop_disable_blocking.js

-              {content}
-            </EuiButton>
+            {(provided) => (
+              <EuiPanel className="custom" paddingSize="s">


Is the className "custom" necessary?

I took a few passes adding/removing the "custom" class and can't see any difference. I'll go ahead and remove it as part of the next code push.

src-docs/src/views/drag_and_drop/drag_and_drop_example.js

kibanamachine · 2022-01-28T19:18:23Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5568/

…rns.

kibanamachine · 2022-01-28T20:38:31Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5568/

elizabetdev

Thanks, @1Copenut for making the changes.

I tested again in Safari, Chrome, and Firefox. I also looked at the code changes and LGTM! 🎉

Revising drag and drop for accessible interactive and complex patterns.

478b58d

1Copenut added accessibility WCAG A labels Jan 26, 2022

1Copenut self-assigned this Jan 26, 2022

1Copenut requested review from cee-chen, elizabetdev and thompsongl January 26, 2022 22:59

1Copenut added 2 commits January 27, 2022 10:00

Adding documentation and changelog entry for drag and drop patterns

de4bae5

* Adding documentation for custom and interactive patterns props. * Adding changelog entry.

Fixing changelog merge conflict.

1ed2ab3

1Copenut marked this pull request as ready for review January 27, 2022 16:06

1Copenut commented Jan 27, 2022

View reviewed changes

src-docs/src/views/drag_and_drop/drag_and_drop_complex.js Outdated Show resolved Hide resolved

src-docs/src/views/drag_and_drop/drag_and_drop_disable_blocking.js Outdated Show resolved Hide resolved

src/components/drag_and_drop/draggable.tsx Show resolved Hide resolved

cee-chen reviewed Jan 27, 2022

View reviewed changes

src/components/drag_and_drop/_draggable.scss Outdated Show resolved Hide resolved

cee-chen reviewed Jan 27, 2022

View reviewed changes

src/components/drag_and_drop/draggable.tsx Outdated Show resolved Hide resolved

Refactoring to remove demo-only CSS and prop.

4d341a2

Adding snapshot test and better comment for a new prop.

a61a358

1Copenut requested a review from cee-chen January 27, 2022 22:53

cee-chen reviewed Jan 27, 2022

View reviewed changes

src/components/drag_and_drop/draggable.test.tsx Outdated Show resolved Hide resolved

src/components/drag_and_drop/draggable.tsx Show resolved Hide resolved

src/components/drag_and_drop/draggable.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Jan 27, 2022

View reviewed changes

1Copenut added 2 commits January 28, 2022 08:19

Small updates to add comments, clearer prop name.

27c1d54

Reformatting inline comment on draggable.tsx

a25e6e8

1Copenut requested a review from cee-chen January 28, 2022 14:25

cee-chen approved these changes Jan 28, 2022

View reviewed changes

src/components/drag_and_drop/draggable.tsx Outdated Show resolved Hide resolved

Updating TS comments for hasInteractiveChildren prop, for clarity.

490670c

elizabetdev suggested changes Jan 28, 2022

View reviewed changes

Refactored standard DIV to EuiPanel for mconsistent interactive patte…

0952f2b

…rns.

1Copenut requested a review from elizabetdev January 28, 2022 19:57

elizabetdev approved these changes Jan 31, 2022

View reviewed changes

1Copenut merged commit f357404 into elastic:main Jan 31, 2022

1Copenut deleted the feature/tpierce-eui-5294-dnd branch January 31, 2022 16:44

breehall mentioned this pull request Feb 8, 2022

Upgrade EUI to v48.1.1 elastic/kibana#125023

Merged

		<EuiPanel className="custom" paddingSize="m">
		<EuiFlexGroup alignItems="center" gutterSize="l">

Conversation

1Copenut commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Design change

Checklist

Uh oh!

kibanamachine commented Jan 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Jan 27, 2022

Uh oh!

Uh oh!

Uh oh!

cee-chen commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thompsongl commented Jan 27, 2022

Uh oh!

kibanamachine commented Jan 27, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cee-chen Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

1Copenut Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 27, 2022

Uh oh!

kibanamachine commented Jan 28, 2022

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elizabetdev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elizabetdev Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

1Copenut Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

elizabetdev Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

1Copenut Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Jan 28, 2022

Uh oh!

kibanamachine commented Jan 28, 2022

Uh oh!

elizabetdev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

1Copenut commented Jan 26, 2022 •

edited

Loading

cee-chen commented Jan 27, 2022 •

edited

Loading

elizabetdev left a comment •

edited

Loading