feat(kit): InputSlider refactor to new Textfield
#10288
Conversation
nsbarsukov
commented
Jan 30, 2025
•
nsbarsukov
commented
Failed tests ❌Before (main) ← Diff → After (local)tests-kit-slider-slider.pw-49273-e-value-formController-1000-chromium-retry2/08-slider-formControl-1000.diff.png
tests-kit-slider-slider.pw-58856-ge-value-formController-750-chromium-retry2/08-slider-formControl-750.diff.png
tests-kit-slider-slider.pw-58d4c-ge-value-formController-500-chromium-retry2/08-slider-formControl-500.diff.png
tests-kit-slider-slider.pw-b2ca8-ange-value-formController-0-chromium-retry2/08-slider-formControl-0.diff.png
tests-kit-input-slider-inp-64258-stfix-input-is-NOT-focused--chromium-retry2/input-slider-content-visible.diff.png
tests-demo-demo.pw-Demo-components-input-slider-chromium-retry2/components-input-slider/1.diff.png
tests-demo-demo.pw-Demo-components-slider-chromium-retry2/components-slider/3.diff.png
tests-demo-demo.pw-Demo-components-input-slider-webkit-retry2/components-input-slider/1.diff.png
tests-demo-demo.pw-Demo-components-slider-webkit-retry2/components-slider/3.diff.png
tests-kit-input-slider-inp-64258-stfix-input-is-NOT-focused--webkit-retry2/input-slider-content-visible.diff.png
tests-kit-slider-slider.pw-49273-e-value-formController-1000-webkit-retry2/08-slider-formControl-1000.diff.png
tests-kit-slider-slider.pw-58856-ge-value-formController-750-webkit-retry2/08-slider-formControl-750.diff.png
tests-kit-slider-slider.pw-58d4c-ge-value-formController-500-webkit-retry2/08-slider-formControl-500.diff.png
tests-kit-slider-slider.pw-b2ca8-ange-value-formController-0-webkit-retry2/08-slider-formControl-0.diff.png
(updated for commit fa0f7a9) |
|
Visit the preview URL for this PR (updated for commit fa0f7a9): https://taiga-previews-demo--pr10288-new-input-slider-demo-dzrh1ind.web.app (expires Fri, 14 Feb 2025 14:28:33 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 73dddc3c665194f3e11f18c16aeb71af4c289c37 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10288 +/- ##
==========================================
- Coverage 65.65% 65.48% -0.17%
==========================================
Files 1246 1249 +3
Lines 16308 16388 +80
Branches 2352 2383 +31
==========================================
+ Hits 10707 10732 +25
- Misses 5391 5490 +99
+ Partials 210 166 -44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
BundleMonFiles updated (2)
Unchanged files (3)
Total files change +1.32KB +0.2% Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
nsbarsukov
force-pushed
the
new-input-slider
branch
2 times, most recently
from
8d5e2c5 to
8c0c7df
Compare
nsbarsukov
changed the title
InputSliderInputSlider refactor to new Textfield
nsbarsukov
force-pushed
the
new-input-slider
branch
2 times, most recently
from
18e15b8 to
55b4fb8
Compare
| protected onStep(coefficient: number): void { | ||
| const slider = this.slider(); | ||
|
|
||
| if (this.textfield.focused() && slider) { |
There was a problem hiding this comment.
When can it not be focused? Why do we use slider value? Can we just use value?
There was a problem hiding this comment.
When can it not be focused?
You are right. I'll drop this check
Why do we use slider value? Can we just use value?
Unfortunately, it is required for [keySteps]-case
| private readonly slider = signal<TuiSliderComponent | null>(null); | ||
| protected readonly textfield = inject(TuiTextfieldComponent); | ||
|
|
||
| protected min = computed(() => this.inputNumber()?.min() ?? 0); |
There was a problem hiding this comment.
I wish we could get rid of this trick somehow, since because we use computed here we have a mess with types inside slider
| ); | ||
|
|
||
| return (newValuePercentage * (max - min)) / 100 + min; | ||
| public set keySteps(steps: TuiKeySteps) { |
There was a problem hiding this comment.
We can maybe inject the other directive and use its transformer instead of this setter?
There was a problem hiding this comment.
It should be overwritten on every input change.
I could use effect instead.
Is this solution is better
private readonly keyStepsBase = inject(TuiSliderKeyStepsBase);
protected readonly $ = effect(
() => (this.transformer = this.keyStepsBase.transformer()),
);than this one ?
private readonly slider = inject<TuiSliderComponent>(
forwardRef(() => TuiSliderComponent),
);
@Input()
public set keySteps(steps: TuiKeySteps) {
this.transformer = tuiCreateKeyStepsTransformer(steps, this.slider);
}|
|
||
| public get max(): number { | ||
| return Number(this.el.max || 100); | ||
| return (range && (this.value - this.min()) / range) || 0; |
There was a problem hiding this comment.
Why change that part? Seemed easier to read as a one-liner to me :)
There was a problem hiding this comment.
Native slider behaves in the such way
taiga-ui/projects/kit/components/slider/test/slider.spec.ts
Lines 176 to 189 in 55b4fb8
There was a problem hiding this comment.
No, I mean why moving range in a separate const? Why not:
return (this.value - this.min()) / (this.max() - this.min()) || 0;There was a problem hiding this comment.
It is not interchangeable code.
If this.max() - this.min() return 0 => all division returns Infinity.
nsbarsukov
force-pushed
the
new-input-slider
branch
2 times, most recently
from
ddca910 to
d27417d
Compare
nsbarsukov
changed the title
InputSlider refactor to new TextfieldInputSlider refactor to new Textfield
Playwright test results
Details
Failed testschromium › tests/deep/deep-select.pw.spec.ts › Deep / Select › /components/input-slider Flaky testswebkit › tests/demo/demo.pw.spec.ts › Demo › /layout/card-medium Skipped testswebkit › tests/addon-commerce/input-card-group.pw.spec.ts › InputCardGroup › Examples › input card grouped with validation |
nsbarsukov
force-pushed
the
new-input-slider
branch
from
8ba4653 to
c670cf8
Compare
nsbarsukov
requested review from
MarsiBarsi,
waterplea,
vladimirpotekhin and
mdlufy
and removed request for
a team
nsbarsukov
force-pushed
the
new-input-slider
branch
from
13ab79f to
d1b3ccf
Compare
