-
Notifications
You must be signed in to change notification settings - Fork 172
Implement forceSelection attribute #240
base: dev
Are you sure you want to change the base?
Conversation
Hi @gRoussac, Thanks for the PR, it's a nice addition. |
Yes please check if blur event is also behaving accordingly (not going through handleSelection if there was no selection) Thanks ! success ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long time it took me to review, it's a good idea but it requires some more work before it's completed.
src/directives/ctr-input.ts
Outdated
@@ -99,7 +100,9 @@ export class CtrInput { | |||
if (this.completer.hasHighlighted()) { | |||
event.preventDefault(); | |||
} | |||
this.handleSelection(); | |||
if (!this.forceSelection || this.completer._hasHighlighted) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
completer._hasHighlighted
is a private member please use completer.hasHighlighted()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the check can be done inside handleSelection
which will prevent the code duplication here and in KEY_TAB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not remember precisely what I was trying to do with the TAB, I guess maybe leaving the menu opened on TAB but this would prevent from switching to a next input in the form, which is pretty much what should happen. Otherwise you need to tqke your mouse to leave the field, and would be some kind of autofocus on TAB.
So I guess only ENTER key should be concerned.
README.md
Outdated
@@ -112,6 +112,7 @@ Add the following to `System.js` map configuration: | |||
|overrideSuggested|If true will override suggested and set the model with the value in the input field.|boolean|No|false| | |||
|openOnFocus|If true will open the dropdown and perform search when the input gets the focus.|boolean|No|false| | |||
|fillHighlighted|If true will set the model with the value in the input field when item is highlighted.|boolean|No|true| | |||
|forceSelection|If true will prevent from closing dropdown on Enter and Tab keys down if no selection was made.|boolean|No|true| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default should be false
@@ -1,6 +1,6 @@ | |||
<div class="completer-holder" ctrCompleter> | |||
<md-input-container class="completer-input"> | |||
<input mdInput ctrInput="clearSelected=clearSelected; overrideSuggested=overrideSuggested; fillHighlighted=fillHighlighted" [(ngModel)]="searchStr" | |||
<input mdInput ctrInput="clearSelected=clearSelected; overrideSuggested=overrideSuggested; fillHighlighted=fillHighlighted; forceSelection=forceSelection" [(ngModel)]="searchStr" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'll be better to change one of the nativeCmp
demos to show how it's working
src/components/completer-cmp.ts
Outdated
@@ -135,6 +135,7 @@ export class CompleterCmp implements OnInit, ControlValueAccessor, AfterViewChec | |||
@Input() public openOnFocus = false; | |||
@Input() public initialValue: any; | |||
@Input() public autoHighlight = false; | |||
@Input() public forceSelection = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forceSelection
should also be used in the component html template [forceSelection]="forceSelection"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a confusion thinking completer-cmp-md.html was the template. I was rushing at that time ×)
@gRoussac when |
…ection Conflicts: src/directives/ctr-input.ts
On blur forceSelection is not checked and should not be. You are just leaving the input, I do not really get was I was trying to say/secure with this blur, seems to work correctly with forceSelection enabled ( ie: close the menu, no point in preventing going out of the component here) (clearUnselected option is I guess was I was missing for the blur part, which enables to force a selection in items or nothing; i.e having forceSelection to true and clearUnselected to false might not be very consistent in ux) (Btw clearUnselected option seems to bug with ESC key fyi) |
Not sure I pulled the right branch. This PR is update to master branch though |
Is it possible for someone to merge this PR with the master branch, to fix the annoying Enter issue! Thanks |
Hi,
Thanks for your nice work on this component.
As user story, we would like to prevent the closing of the dropdown menu on hitting "Enter"
or "Tab"when nothing was selected in the dropdown. We could not find an option corresponding to that case or maybe we missed it in the doc, if so please any advise appreciated.The pull request may not be perfect but you get the point I hope.
Best regards,
Greg