SelectionList bugs #1731
SelectionList bugs #1731
Conversation
benjamincharity
commented
Oct 2, 2019
•
benjamincharity
commented
- cursor correct on hover. closes SelectionList: Should have correct hover styles #1730
- form field hover now has correct styles
- focused options are now scrolled into view. closes SelectionList: Option should scroll into view when focused with the keyboard #1727
- first option focused by default. closes SelectionList: The first option should always be selected when querying #1729
ISSUES CLOSED: #1730
ISSUES CLOSED: #1730
benjamincharity
force-pushed
the
1730-selectionlist-hover
branch
from
28ad2a6
to
80b2cef
Compare
Codecov Report
@@ Coverage Diff @@
## release #1731 +/- ##
===========================================
- Coverage 98.82% 98.77% -0.05%
===========================================
Files 134 134
Lines 5711 5729 +18
Branches 976 982 +6
===========================================
+ Hits 5644 5659 +15
- Misses 67 69 +2
- Partials 0 1 +1
Continue to review full report at Codecov.
|
| @@ -73,6 +72,9 @@ export const TS_SELECTION_LIST_SCROLL_STRATEGY_FACTORY_PROVIDER = { | |||
| useFactory: TS_SELECTION_LIST_SCROLL_STRATEGY_FACTORY, | |||
| }; | |||
|
|
|||
| // The max height of the select's overlay panel | |||
| export const SELECTION_LIST_PANEL_MAX_HEIGHT = 256; | |||
There was a problem hiding this comment.
Is it the max height regardless current window/screen size?
There was a problem hiding this comment.
yep, just like select and autocomplete.
| // Try to use the 2nd option in case the first option is blank or a filter etc. Fall back to the first item if needed. | ||
| const options = this.selectionListPanel.options.toArray(); | ||
| const option = options[1] || options[0]; | ||
| return option.elementRef.nativeElement.offsetHeight; |
There was a problem hiding this comment.
Can the option height be defined by consumers?
There was a problem hiding this comment.
It pretty much already is. We are checking the rendered height, so if the consumer puts in 200px tall content, that's what it will be. Our default height is just created by our CSS. So the template drives the height.
| @@ -752,7 +770,8 @@ export class TsSelectionListTriggerDirective<ValueType = string> implements Cont | |||
| // Create a new stream of panelClosingActions, replacing any previous streams that were created, and flatten it so our stream only | |||
| // emits closing events... | |||
| switchMap(() => { | |||
| this.resetActiveItem(); | |||
| // Focus the first option when options change | |||
| this.selectionListPanel.keyManager.setActiveItem(0); | |||
There was a problem hiding this comment.
Does resetActiveItem also do the same thing - set the first option active? although, this way is clearer...
There was a problem hiding this comment.
resetActiveItem clears the selection completely. Which mentally seems fine until you start using the component. The UX of being able to just type to filter and hit enter is just better imo
| @@ -965,4 +965,24 @@ describe(`TsSelectionListComponent`, function() { | |||
| expect(fixture.componentInstance.clicked).toHaveBeenCalled(); | |||
| })); | |||
|
|
|||
| // Note: Simply dispatching the arrow down key on the trigger repeatedly did not wor | |||
There was a problem hiding this comment.
your keyboard might have cut k out...
| tick(1000); | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(instance.panel.keyManager.activeItemIndex).toEqual(0); |
There was a problem hiding this comment.
This however doesn't test using arrow key that option list should scroll when list is longer than viewport height - do notice that arrow key doesn't work.... we do have other specs that using arrow down key, right? wonder how they're different..
There was a problem hiding this comment.
I wouldn't expect it to? This test is called should focus first option when the options collection changes and that is all it tests. There is a TODO test above this to cover that called should update panel scroll position when focusing an out-of-view option
There was a problem hiding this comment.
Okay, I didn't see a todo and that's all the specs you added so just wanted to point it out...
Cause we are unfortunately under the gun currently to get some items out, so I'm leaving several |
ISSUES CLOSED: #1729
benjamincharity
force-pushed
the
1730-selectionlist-hover
branch
from
b526249
to
fe14755
Compare
