Add option for not trimming input value #19
Conversation
We add a "trim" option that is by default set to "true", in order for the component to stay backward compatible. However, when this property is set to "false", the value is the one directly entered by the user, with all whitespaces left untouched.
LukaszKokot
changed the title
There was a problem hiding this comment.
This looks good to me. I'm not overly familiar with this code, but from what I do know, I'd be happy bringing it in and verifying in Looker.
Looks like you've essentially built angular-ui#2064, we should discuss whether this is something we should open back into the original repo.
| @@ -1139,7 +1140,7 @@ describe('ui-select tests', function () { | |||
| var el = createUiSelect(); | |||
| expect(el.find('.ui-select-choices-group .ui-select-choices-group-label').map(function () { | |||
| return this.textContent; | |||
| }).toArray()).toEqual(["Foo", "Baz", "bar"]); | |||
There was a problem hiding this comment.
Yeah that is weird... @LukaszKokot do you know why this changed?
|
The last few commits are me 🤦♂ . This is the angular ui-select, got it. Disregard my last comment. |
|
It was really a quick fix, no worries @rlm1023. Thanks for the review! |
|
@RichardCzechowski Looks like there already is a PR (cf. angular-ui#2157) open since August 2018. Not sure the original repo is really maintained anymore. |
|
We really might want someone like @stalbot 's opinion here. A little worried with how this could fallout. |
|
Who's on paternity leave. Maybe @lameyer ? |
|
uh oh I've been summoned 😂 I'll take a look |
|
@rlm1023 @lameyer @RichardCzechowski For the record, I have a PR using this version of |
There was a problem hiding this comment.
Since this is just adding an option, and by default does the exact same thing, I feel pretty comfortable with this. @LukaszKokot if you can confirm with manual testing when you have helltool with this change the other input boxes will still trim spaces, then I feel like we're good
| @@ -1139,7 +1140,7 @@ describe('ui-select tests', function () { | |||
| var el = createUiSelect(); | |||
| expect(el.find('.ui-select-choices-group .ui-select-choices-group-label').map(function () { | |||
| return this.textContent; | |||
| }).toArray()).toEqual(["Foo", "Baz", "bar"]); | |||
There was a problem hiding this comment.
Yeah that is weird... @LukaszKokot do you know why this changed?
|
@lameyer this is for fixing a unit test failing from the last commit to master. Please see Rob's comment, cf. #19 (comment) :-) |
|
Sorry my "disregard comment" comment was about this one:
Not that unit test changing. Still want to make sure we understand why that changed. |
|
So, I ran the tests on master, and that test I fixed was there, so something was wrong before my PR. And the last |
|
So this test was broken on master as well you're saying? That's probably fine then 🤷♀ |
We add a "trim" option that is by default set to "true", in order for the
component to stay backward compatible. However, when this property is set
to "false", the value is the one directly entered by the user, with all
whitespaces left untouched.