Fix: replaced ember input with html input in OSS::InputContainer#533
Fix: replaced ember input with html input in OSS::InputContainer#533OwenCoogan wants to merge 5 commits intomasterfrom
Conversation
OwenCoogan
force-pushed
the
oc/VEL-5450
branch
from
cfe2b19 to
7a5a610
Compare
OwenCoogan
force-pushed
the
oc/VEL-5450
branch
from
095037b to
6d6a2ae
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Miexil
left a comment
Miexil
left a comment
There was a problem hiding this comment.
Lots of questions - but nothing is explained in the PR so 🤷
Inputs are the core of our application, if we mess things up here, bugs & defects will rain.
| get onChangeHandler() { | ||
| return (value: string | null) => this.validateInput(value); | ||
| } | ||
|
|
||
| @action | ||
| validateInput(): void { | ||
| this.regexError = ''; | ||
| this.args.onChange?.(this.args.value ? this.args.value.toLowerCase() : this.args.value); | ||
| validateInput(value: string | null): void { |
There was a problem hiding this comment.
I'm not sure I understand these changes 🤔
Why do we need to use a getter for the validateInput ?
and why is the regexError reset logic moved ?
| autocomplete={{this.autocomplete}} | ||
| class="upf-input" | ||
| {{on "keyup" (fn this._onChange @value)}} | ||
| {{on "input" this.onInput}} |
There was a problem hiding this comment.
Why do we need to add an on input listener ?
|
|
||
| @action | ||
| onKeyUp(event: KeyboardEvent): void { | ||
| if (event.key === 'Backspace' && this.args.value === null) { |
There was a problem hiding this comment.
Again, why change the logic here ?
| disabled={{or this.isMinDisabled @disabled}} {{enable-tooltip title=this.minTooltipTitle placement="top"}} /> | ||
| {{! template-lint-disable no-triple-curlies}} | ||
| <OSS::InputContainer @value={{this.localValue}} @onChange={{this.checkUserInput}} @disabled={{@disabled}} style={{{this.dynamicWidth}}} | ||
| <OSS::InputContainer @value={{this.localValue}} @onChange={{fn this.checkUserInput}} @disabled={{@disabled}} style={{{this.dynamicWidth}}} |
| if (value !== undefined) { | ||
| this.localValue = Number(value); | ||
| } |
| await fillIn('input', 'fakedomai'); | ||
| await typeIn('input', 'n'); | ||
| assert.true(this.onChange.calledOnceWithExactly('fakedomain', true)); | ||
| assert.true(this.onChange.lastCall.calledWith('fakedomain', true)); |
There was a problem hiding this comment.
If the test needs to be updated, it means the logic has changed somewhere - is this expected ?
There was a problem hiding this comment.
Same as the other locations, why are we making these changes ?
| @action | ||
| onKeyUp(event: KeyboardEvent): void { | ||
| if (event.key === 'Backspace' && this.args.value === null) { | ||
| this.args.onChange?.(null); |
There was a problem hiding this comment.
You are not afraid about 2 possibles calls of this.args.onChange in the component if we are using 2 different events?
| (this.sliderOptions.max - this.sliderOptions.min), | ||
| 1 | ||
| ); | ||
| return Math.min(Math.max(convertedValue + correction - min, 0) / (max - min), 1); |
There was a problem hiding this comment.
Why base the min / max value on this.args.max/min?
6 participants
What does this PR do?
https://www.loom.com/share/06028ccaa77b4edcb88b9c315d522b94
Related to: #
What are the observable changes?
Good PR checklist