Data entry lookup #1220
Data entry lookup #1220
Conversation
Fixed typo of SkyModalConfigurationInterface and exporting in default…
Merge latest
…amsey/skyux2 into data-entry-lookup
…n multiple are selected. Update control to hide menu when there is no (or insufficent) search criteria.
…e menu click event fired.
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: bc8fb5e (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 5dcd3cd (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 6b39a13 (Please note that this is a fully automated comment.) |
Codecov Report
@@ Coverage Diff @@
## master #1220 +/- ##
=======================================
Coverage 100% 100%
=======================================
Files 370 372 +2
Lines 6796 7046 +250
Branches 874 938 +64
=======================================
+ Hits 6796 7046 +250
Continue to review full report at Codecov.
|
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 07b61c2 (Please note that this is a fully automated comment.) |
Blackbaud-ToddRoberts
requested review from
Blackbaud-SteveBrush,
Blackbaud-ToddRoberts and
Blackbaud-PaulCrowder
| font-style: italic; | ||
| } | ||
|
|
||
| .sky-lookup-selected-item { |
There was a problem hiding this comment.
Selected items in multiselect mode should be styled the same as selected filter summary items https://github.com/blackbaud/skyux2/blob/master/src/modules/filter/filter-summary-item.component.scss
|
|
||
| .sky-lookup-menu-item { | ||
| @include sky-dropdown-item(); | ||
| &.sky-lookup-menu-item-focused { |
There was a problem hiding this comment.
Should use the regular dropdown item styling for focus, which just uses the browser default
There was a problem hiding this comment.
With this control, focus as nuanced meaning:
-
There is the concept of an 'active item' which represents the current item which will be selected (if you hit enter or leave for auto-complete). You can change this item by using the up and down arrows.
-
If you attempt to move focus out of the control, it will collapse the menu and resolve the pending text. This means you can't give the menu focus.
-
If you hover, you will get standard dropdown styling.
We need to differentiate between active item and hover visually for usability. I'm going to rename the css class from sky-lookup-menu-item-focus to sky-lookup-menu-item-active to make it more clear.
There was a problem hiding this comment.
To differentiate the active item, the best analogue we currently have is on the Sort component, which uses background $sky-background-color-selected and the $sky-emphasized font styling. I think that would work here, the background color is quite close to the hover gray but the bold/larger text will make it stand out.
There was a problem hiding this comment.
I would caution against altering the font weight on active item because we allow consumers of the control to template the menu contents. A likely scenario would be for two lines of content, where the second line was diminished or otherwise styled.
There was a problem hiding this comment.
What options do they have for styling on the templates?
There was a problem hiding this comment.
The consumer of the component can pass a template for menu items, which essentially allows them to do anything within the confines of the menu item. Multiple lines, include images or icons, format data, etc.
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 08e830d (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: f95ba0c (Please note that this is a fully automated comment.) |
|
The approved design specifies the double-backspace to delete the last item in the list, which has the benefit of specifically calling out which item will be removed and avoids accidental removals. It also specifies that delete/backspace be used to delete the focused items. There are a couple of risks in having Enter remove the item:
I do have concerns about discoverability of using backspace/delete here but am inclined to go with the design as approved, and update it later if we hear that it's problematic. There is a version of this in the K12 products and users have figured out the Delete button without much issue. |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 44cf1a2 (Please note that this is a fully automated comment.) |
| @@ -0,0 +1,73 @@ | |||
| <div (document:click)="windowClick()"> | |||
There was a problem hiding this comment.
The preference is to have as much logic included in the component class as possible. We should use a @HostListener('document:click') in the class, instead.
For example: https://stackoverflow.com/questions/40107008/hostlistener-documentclick-angular-2-same-component
| </span> | ||
| </div> | ||
| </div> | ||
| <div class="sky-dropdown-menu"> |
There was a problem hiding this comment.
Did you already try inserting the <sky-dropdown> component in here, instead of injecting its adapter service? If so, what challenges did you encounter? I wrote a dumbed-down version of what I'm talking about in the lookup-adjustments branch: https://github.com/blackbaud/skyux2/blob/lookup-adjustments/src/modules/lookup/lookup.component.html
There was a problem hiding this comment.
When I started the component I asked the same question. I ended up not using the entire dropdown component directly because the overhead didn't add anything beyond more layers to deal with. If we went down that road I'd make the argument that the menu should be a distinct reusable component separate from the dropdown. The adapters exist as separate services and utilities so they can be reused (and obviously tested) independently of the core component they exist for, so it really becomes a philosophical question as to where you draw the dependency. As constructed today,, there is nothing which should prevent this from changing in the future without impacting public backwards compatibility.
| <div class="sky-dropdown-menu"> | ||
| <div *ngFor="let item of results" class="sky-lookup-menu-item" | ||
| [ngClass]="{'sky-lookup-menu-item-active': (item === activeMenuItem)}"> | ||
| <button type="button" |
There was a problem hiding this comment.
For accessibility, we need to include the aria-activedescendant attribute when selecting individual items, much like Angular Material's autocomplete: https://material.angular.io/components/autocomplete/overview#accessibility
There was a problem hiding this comment.
aria-activedescendant is a really interesting question. According to the documentation, it should be set to the id of the item in the list selected - but the primary purpose of that is when it doesn't otherwise match the contents of the button. In our case, they will be the same. We don't use ids to track items, so a best I would just put the string of the innerHtml which would seem to be unhelpful.
| this.closeMenu(); | ||
| } | ||
|
|
||
| public keydown(event: KeyboardEvent) { |
There was a problem hiding this comment.
There are discussions internally about whether this keyboard behavior should be "fixed" in the Dropdown Module (using the arrow keys, etc.). I'll get back to you on this ASAP.
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: ac0b448 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 5ffd20f (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 4fb0c2b (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 94ff254 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: fa58206 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: c39dd83 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 238ffcb (Please note that this is a fully automated comment.) |
|
Closing this in favor of a three-part contribution:
|
#1196