typeahead improvement - çlear ('x') button which appears after some option selected from suggestions #4483
Conversation
|
I'm fine with this as is I think, but this needs some tests to ensure it is clearing. If this gets rebased, I'd be willing to merge this for 3.0.0, only to mitigate risk of it being a breaking change for others. |
|
|
||
| scope.positionClearBtn = $position.position(element); | ||
| scope.positionClearBtn.top += element.prop('offsetHeight')/4; | ||
| scope.positionClearBtn.left += element.prop('offsetWidth')-20; |
There was a problem hiding this comment.
For these two lines, add spacing for the math operations, i.e. foo / 4 and foo - 20.
|
@shootermv what's with all the indents? Can you please clean all that up? It makes it challenging to review this. |
|
Hi Adam. On Thu, Aug 18, 2016 at 11:23 PM, Adam Gordon [email protected]
|
| return element.find('.glyphicon-remove'); | ||
| }; | ||
|
|
||
|
|
|
@shootermv, You still have some spacing issues (auto formatter from your IDE?) around extra blank lines and trailing whitespace. Also, please squash your commits. In the future, you can avoid this by reusing your original commit via |
|
Thanks a lot, removed some more spaces problem as much as i can see, On Fri, Aug 19, 2016 at 3:17 AM, Adam Gordon [email protected]
|
feat(typeahead): removed all extra indents and $scope.$digest feat(typeahead): removed some extra spaces & indents feat(typeahead): removed some extra spaces & indents 2
|
Also tried to squash some commits into recent On Fri, Aug 19, 2016 at 12:29 PM, Moshe Vilner [email protected] wrote:
|
| scope.positionClearBtn = $position.position(element); | ||
| scope.positionClearBtn.top += element.prop('offsetHeight') / 4; | ||
| scope.positionClearBtn.left += element.prop('offsetWidth') - 20; | ||
|
|
There was a problem hiding this comment.
remove the extra blank lines here, please
| @@ -351,6 +362,9 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.debounce', 'ui.bootstrap | |||
| var model, item; | |||
|
|
|||
| selected = true; | |||
| //indicator that some choise selected for clear button | |||
There was a problem hiding this comment.
a couple things:
- prefer end-of-line comments as opposed to comments that take their own line. especially when short
- it should be "choice"
- please put a space between the
//and the first character of the comment - remove the blank line between this variable declaration and the line for
locals[parserResult... - please pick a better variable name than
selectedM- variable names should describe what it is.
|
@shootermv a bunch of little things i've commented on. you should google how to squash your commits. also, it doesn't look like you followed my instructions about how to reuse your existing commit and then do a force push? |
|
Hi, in order to squash all my changes into one commit i deleted and On Fri, Aug 19, 2016 at 8:55 PM, Adam Gordon [email protected]
|
I'm newbie - so may be my code will have a lot of issues (for example may be it better to make this feature optional) , so please write me what to improve and will try my best.
Thanks