Contrib > Lookup Component (Part 1) > Dropdown A11y Adjustments #1386
Conversation
Blackbaud-SteveBrush
requested review from
Blackbaud-BobbyEarl and
Blackbaud-PaulCrowder
Codecov Report
@@ Coverage Diff @@
## master #1386 +/- ##
=======================================
Coverage 100% 100%
=======================================
Files 370 370
Lines 6789 6942 +153
Branches 874 892 +18
=======================================
+ Hits 6789 6942 +153
Continue to review full report at Codecov.
|
Blackbaud-SteveBrush
changed the title
| @@ -2,11 +2,5 @@ | |||
| { | |||
| // Columns at which to show vertical rulers | |||
| "editor.rulers": [100], | |||
|
|
|||
| // Controls after how many characters the editor will wrap to the next line. Setting this to 0 turns on viewport width wrapping (word wrapping). Setting this to -1 forces the editor to never wrap. | |||
| "editor.wordWrap": "wordWrapColumn", | |||
There was a problem hiding this comment.
Removing some outdated settings, as well as the word wrap requirement.
Blackbaud-SteveBrush
changed the title
|
Below are a couple things I quickly noticed while looking at the demo page based on the components you mentioned this PR affecting:
|
| </sky-row> | ||
|
|
||
| <h3> | ||
| Showing a dropdown on mouse hover |
There was a problem hiding this comment.
Is there a SKY UX pattern for displaying a dropdown on mouseover? If not, we may not want to have this example because consumers may infer this is something that they should do in certain scenarios. If so, we should document why a consumer would want to show a dropdown on mouseover.
|
|
||
| public openDropdown(event: MouseEvent) { | ||
| this.sendMessage(SkyDropdownMessageType.Open); | ||
| event.preventDefault(); |
There was a problem hiding this comment.
It's best practice to not reference browser-specific events like MouseEvent in an Angular component (https://angular.io/guide/user-input#passing-event-is-a-dubious-practice). You can get the same behavior in the template by adding ; false after the click handler:
<button type="button" (click)="openDropdown(); false"></button>
|
|
||
| public openDropdown(event: MouseEvent) { | ||
| this.sendMessage(SkyDropdownMessageType.Open); | ||
| event.preventDefault(); |
There was a problem hiding this comment.
Referencing a browser event in an Angular component is not best practice (https://angular.io/guide/user-input#passing-event-is-a-dubious-practice). You should be able to achieve the same result by adding ; false to the end of the click handler in your template:
<button type="button" (click)="openDropdown(); false"></button>
| </sky-row> | ||
|
|
||
| <h3> | ||
| Showing a dropdown on mouse hover |
There was a problem hiding this comment.
Is there a SKY UX pattern that shows a dropdown on hover? If not, we may want to remove this example because it may make the consumer wonder if this is something they should be doing. If so, we should document when this is appropriate behavior.
There was a problem hiding this comment.
This isn't something we want to promote. In general things should appear on click, anything that's done on hover would be an exception we'd want to evaluate on a case by case basis in a design review for that app/feature.
|
|
||
| public ngOnInit() { | ||
| if (this.messageStream) { | ||
| this.messageStream |
There was a problem hiding this comment.
This is an interesting way of handling interactions with the dropdown. Should we also consider making this the only way the dropdown changes state? For instance, internal to the component we have calls to this.openDropdown(). Should we not push messages to the message stream instead? That would have the added benefit of the consumer handling events that happen internal to the component, such as when the dropdown is opened, closed, etc.
|
I'm approving this. @Blackbaud-BobbyEarl did you still have things you wanted addressed? |
|
Yes, I'd followed-up directly with @Blackbaud-SteveBrush, but I was still seeing strange behavior when adding more than 11 tabs, causing the dropdown to be used. Whether the dropdown was fullscreen or normal altered back and forth with each click. Were you able to duplicate that Steve? |
|
@Blackbaud-BobbyEarl Yes, I was able to replicate the behavior. I've made some changes to that effect. Do you mind running another check? Thank you! |
Adjustments, new features:
skyPopoverAlignmentinput, to allow horizontal alignment of dropdowns ("left", "center", or "right").Components affected by change:
Related pull request: #1220
Documentation pull request: blackbaud/skyux2-docs#19