Add keyboard accessible dragondrop to tiles #1815

blackbaud-conorwright · 2018-07-11T14:46:13Z

Added missing aria-label for the tile drag handle.
Implemented arrow key operation of tile position within and between columns

Resolves: #1371 and #1372

codecov-io · 2018-07-11T16:51:28Z

Codecov Report

Merging #1815 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1815   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         422     424    +2     
  Lines        8993    9056   +63     
  Branches     1332    1355   +23     
======================================
+ Hits         8993    9056   +63

Impacted Files	Coverage Δ
...les/tiles/tile-dashboard/tile-dashboard.service.ts	`100% <100%> (ø)`	⬆️
...ules/tiles/tile-dashboard/tile-dashboard.module.ts	`100% <100%> (ø)`	⬆️
src/modules/tiles/tile/tile.module.ts	`100% <100%> (ø)`	⬆️
...s/tiles/tile-dashboard/tile-dashboard.component.ts	`100% <100%> (ø)`	⬆️
src/modules/tiles/tile/tile.component.ts	`100% <100%> (ø)`	⬆️
src/modules/radio/radio.component.ts	`100% <0%> (ø)`	⬆️
src/modules/flyout/flyout.module.ts	`100% <0%> (ø)`	⬆️
src/modules/toast/toast.module.ts	`100% <0%> (ø)`	⬆️
src/modules/log/log.service.ts	`100% <0%> (ø)`	⬆️
src/modules/log/log.module.ts	`100% <0%> (ø)`	⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 949cfd1...279c817. Read the comment docs.

…ssible-dragondrop-to-tiles

Blackbaud-AlexKingman · 2018-07-30T21:26:18Z

@blackbaud-conorwright this is looking good. Couple questions:

I noticed the click target for the move handle is shaped differently than the other two buttons. I'm assuming this is to make it easier to grab with the mouse? If so, maybe that's OK then. If not, should we look at modifying padding/margins to make that match the others?
Is using arrow keys to move an element an established convention for folks with assistive tech? For example, will they know if they hear an element called "Move" that the next thing they should do is use the arrow keys? I only ask this because I wonder if there's a way to instruct a user what to do in this scenario - maybe the label could say "Move with arrow keys" or perhaps there's another aria tag to expand upon this?

blackbaud-conorwright · 2018-07-31T14:05:26Z

Hey @Blackbaud-AlexKingman
I'll run the label issue by Matt, it may not be intuitive enough. I think the ArrowKey use is common enough though, just may need a better descriptor.
For 1 though, I'm not sure what you mean here. I don't believe I altered the handle appearance and at least on my screen it looks to be the same size and spacing as the other 2 icons in the demo. Do you mean that it is a different icon or am I missing the issue?

Blackbaud-AlexKingman · 2018-07-31T15:11:06Z

Here's what I mean about the focus target being different from the others:

If you take a look at the other buttons, its a nice equal square.

…com:blackbaud/skyux2 into add-keyboard-accessible-dragondrop-to-tiles

Blackbaud-AlexKingman · 2018-08-02T20:43:07Z

@blackbaud-conorwright your click-target changes look good! Any news back from Matt re: label clarification?

blackbaud-conorwright · 2018-08-03T14:24:02Z

Not yet, I think he might be rather busy this week. I may ping him again later today if I don't hear back.

Blackbaud-AlexKingman · 2018-08-07T13:50:48Z

@blackbaud-conorwright I may have found a nice pattern that will improve the accessibility here. Check out "Pattern #1: Sorting a List" from this article.

tl;dr

  <span id="operation" class="assistive-text">
    Press Arrow Key to reorder
  </span>
  ...
  <li role="option" draggable="true" aria-describedby="operation" 
      tabindex="0">
    Ice Cream
  </li>

When tabbing in to the first list-item, it will read,

“Listbox. 1 of X. Ice Cream. Press Arrow Key to reorder.”

What do you think of this @Blackbaud-MattGregg ?

Blackbaud-MattGregg · 2018-08-13T16:08:10Z

I think that pattern looks promising. Looks great with use of Live region and feedback on where item is positioned after changes.
For the tiles scenario, it would need to be tweaked slightly given that example has a different semantic structure. Their example uses the aria-describedby because it's it's an item in a list and already has a label value. The tiles icons elements seems to be missing a role so would probably need one and a text label for example.

src/modules/tiles/tile-dashboard/tile-dashboard.service.spec.ts

Blackbaud-AlexKingman · 2018-09-06T16:56:32Z

src/modules/tiles/tile-dashboard/tile-dashboard.service.ts

+    if (this.config && this.config.layout.multiColumn) {
+      for (let column of this.config.layout.multiColumn) {
+        /*istanbul ignore else */
+        if (column.tiles) {


Just curious - would this be more clear with a .find() instead of a for() loop? I don't have a good answer, just curious about your thoughts.

Oh nice, yeah that would be more efficient 😄
swapped

Blackbaud-AlexKingman · 2018-09-06T17:19:14Z

src/modules/tiles/tile-dashboard/tile-dashboard.service.ts

@@ -159,6 +145,100 @@ export class SkyTileDashboardService {
    }
  }

+  public moveTile(tileCmp: SkyTileComponent, direction: string, tileDescription: string) {


Since this is only utilized via keyboard commands, should we make the method name more specific? Something like moveTileOnKeyDown?

Yeah, that would be a better name. Changing

Blackbaud-AlexKingman · 2018-09-06T17:27:41Z

src/modules/tiles/tile-dashboard/tile-dashboard.service.ts

+        }
+
+        // Move the tile element in the document
+        let tileComponentInstance = this.getTileComponent(tileId);


Maybe being paranoid here, but should we do a null check on tileComponentInstance ? Is there a chance of that ever being null?

Added a check :)
It seems very unlikely since I pass in the component instance but this tie in could be bumped eventually with mismatch between the document and our internal map of the tiles

Blackbaud-AlexKingman · 2018-09-06T17:29:15Z

src/modules/tiles/tile-dashboard/tile-dashboard.service.ts

+        column.tiles = column.tiles.filter(item => item !== tile);
+        this.moveTilesToColumn(this.columns.toArray()[colIndex + operator], [tile]);
+
+        // Report the change in configuration


I see this report feature a couple times in here. Should we encapsulate it to its own method?

I don't think it's really worth it here. All I'm doing is constructing the object and the difference between these two calls are nearly all of the logic.

Blackbaud-AlexKingman · 2018-09-10T15:41:19Z

I like the changes. Much cleaner! Great job. 🚢 🇮🇹!

blackbaud-conorwright added 3 commits July 10, 2018 10:21

added missing label & keyboard accessbile dragndrop

9ae1879

added tests for accessible dnd. Added ArrowKey support

448326a

fixed import reorganization

e35d8d5

blackbaud-conorwright requested a review from Blackbaud-SteveBrush July 11, 2018 14:46

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

53ec73a

blackbaud-conorwright and others added 5 commits July 16, 2018 12:59

Merge remote-tracking branch 'upstream/master' into add-keyboard-acce…

c07c7e7

…ssible-dragondrop-to-tiles

Merge remote-tracking branch 'upstream/master' into add-keyboard-acce…

1f6f078

…ssible-dragondrop-to-tiles

switched to a shorter condition for movetile

d3c2f75

fixed missing coverage in tile service

f10974d

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

f582c87

Blackbaud-SteveBrush assigned Blackbaud-AlexKingman Jul 30, 2018

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

c7eee81

blackbaud-conorwright added 2 commits July 31, 2018 17:01

fixed tile handle outline appearance

f1b25bf

Merge branch 'add-keyboard-accessible-dragondrop-to-tiles' of github.…

3029995

…com:blackbaud/skyux2 into add-keyboard-accessible-dragondrop-to-tiles

Blackbaud-AlexKingman self-requested a review August 1, 2018 12:45

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

4f1111f

Blackbaud-AlexKingman added the Needs: More info label Aug 13, 2018

Blackbaud-AlexKingman assigned blackbaud-conorwright and unassigned Blackbaud-AlexKingman Aug 13, 2018

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

d9bf505

Blackbaud-TrevorBurch removed the Needs: More info label Aug 14, 2018

Blackbaud-AlexKingman unassigned blackbaud-conorwright Aug 17, 2018

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

c957a57

blackbaud-conorwright added sky-tile Severity: Low Status: In progress Severity: High and removed Severity: Low labels Aug 31, 2018

blackbaud-conorwright and others added 3 commits August 31, 2018 17:46

added better accessibility info. Fixed mobile nav

db61457

removed leftover "fit"

c8be2c2

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

4f268ba

blackbaud-conorwright added Status: Ready for review accessibility and removed Status: In progress Severity: High Status: Ready for review accessibility sky-tile labels Sep 6, 2018

Blackbaud-AlexKingman reviewed Sep 6, 2018

View reviewed changes

src/modules/tiles/tile-dashboard/tile-dashboard.service.spec.ts Show resolved Hide resolved

Blackbaud-AlexKingman reviewed Sep 6, 2018

View reviewed changes

blackbaud-conorwright and others added 4 commits September 6, 2018 14:38

addressed PR comments

03cc1c3

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

ed609e8

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

69f5d43

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

a1dc3d2

Blackbaud-AlexKingman approved these changes Sep 10, 2018

View reviewed changes

Merge branch 'master' into add-keyboard-accessible-dragondrop-to-tiles

279c817

blackbaud-conorwright merged commit 2a5ec24 into master Sep 12, 2018

blackbaud-conorwright deleted the add-keyboard-accessible-dragondrop-to-tiles branch September 12, 2018 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add keyboard accessible dragondrop to tiles #1815

Add keyboard accessible dragondrop to tiles #1815

blackbaud-conorwright commented Jul 11, 2018

codecov-io commented Jul 11, 2018 •

edited

Loading

Blackbaud-AlexKingman commented Jul 30, 2018

blackbaud-conorwright commented Jul 31, 2018

Blackbaud-AlexKingman commented Jul 31, 2018

Blackbaud-AlexKingman commented Aug 2, 2018

blackbaud-conorwright commented Aug 3, 2018

Blackbaud-AlexKingman commented Aug 7, 2018

Blackbaud-MattGregg commented Aug 13, 2018

Blackbaud-AlexKingman Sep 6, 2018

blackbaud-conorwright Sep 6, 2018

Blackbaud-AlexKingman Sep 6, 2018

blackbaud-conorwright Sep 6, 2018

Blackbaud-AlexKingman Sep 6, 2018

blackbaud-conorwright Sep 6, 2018

Blackbaud-AlexKingman Sep 6, 2018

blackbaud-conorwright Sep 6, 2018

Blackbaud-AlexKingman commented Sep 10, 2018

Add keyboard accessible dragondrop to tiles #1815

Add keyboard accessible dragondrop to tiles #1815

Conversation

blackbaud-conorwright commented Jul 11, 2018

codecov-io commented Jul 11, 2018 • edited Loading

Codecov Report

Blackbaud-AlexKingman commented Jul 30, 2018

blackbaud-conorwright commented Jul 31, 2018

Blackbaud-AlexKingman commented Jul 31, 2018

Blackbaud-AlexKingman commented Aug 2, 2018

blackbaud-conorwright commented Aug 3, 2018

Blackbaud-AlexKingman commented Aug 7, 2018

Blackbaud-MattGregg commented Aug 13, 2018

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

Blackbaud-AlexKingman commented Sep 10, 2018

codecov-io commented Jul 11, 2018 •

edited

Loading