feat(kit): `Range` use `Slider` inside #1538

nsbarsukov · 2022-03-25T08:15:04Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows Conventional Commits
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Closes #

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

lumberjack-bot · 2022-03-25T08:15:42Z

Pull request was closed ✔️

All saved screenshots (for current PR) were deleted 🗑️

github-actions · 2022-03-25T08:20:06Z

Visit the preview URL for this PR (updated for commit 6824717):

https://taiga-ui--pr1538-range-refactor-ddqp2l3d.web.app

_{(expires Wed, 06 Apr 2022 08:10:10 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

codecov · 2022-03-25T08:21:46Z

Codecov Report

Merging #1538 (6824717) into main (9cdbde9) will decrease coverage by 1.11%.
The diff coverage is 60.24%.

@@            Coverage Diff             @@
##             main    #1538      +/-   ##
==========================================
- Coverage   63.43%   62.32%   -1.12%     
==========================================
  Files         910      696     -214     
  Lines        9953     8496    -1457     
  Branches     1920     1658     -262     
==========================================
- Hits         6314     5295    -1019     
+ Misses       3200     2793     -407     
+ Partials      439      408      -31

Flag	Coverage Δ
addon-charts	`74.69% <ø> (ø)`
addon-doc	`26.15% <ø> (ø)`
addon-editor	`45.87% <ø> (ø)`
addon-mobile	`∅ <ø> (∅)`
addon-table	`81.39% <ø> (ø)`
addon-tablebars	`∅ <ø> (∅)`
cdk	`?`
core	`68.70% <ø> (ø)`
kit	`62.92% <60.24%> (+0.03%)`	⬆️
summary	`62.32% <60.24%> (-1.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
projects/kit/components/range/range.module.ts	`100.00% <ø> (ø)`
projects/kit/components/slider/slider.module.ts	`100.00% <ø> (ø)`
...cts/kit/components/range/range-change.directive.ts	`24.24% <24.24%> (ø)`
...kit/components/slider/slider-readonly.directive.ts	`77.77% <77.77%> (ø)`
projects/kit/abstract/slider/slider.ts	`60.90% <83.33%> (-0.70%)`	⬇️
projects/kit/components/range/range.component.ts	`83.33% <84.37%> (+16.66%)`	⬆️
...ects/kit/components/slider/slider-old.component.ts	`93.75% <100.00%> (+0.89%)`	⬆️
projects/cdk/classes/validation-error.ts
projects/cdk/date-time/date-separator.ts
projects/cdk/utils/math/clamp.ts
... and 215 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cdbde9...6824717. Read the comment docs.

vladimirpotekhin · 2022-03-31T08:36:02Z

projects/kit/components/range/range-change.directive.ts

+     * TODO replace with pointer events (when all supported browsers can handle them).
+     * Dont forget to use setPointerCapture instead of listening all documentRef events
+     */
+    private readonly pointerDown$ = merge(


nit: same stream is used in tuiPan, maybe it makes sense to think about sharing this code 🤔

I agree this directive listens to the same events. But it does different things.

Should we create tuiPointerDown$, tuiPointerMove$, tuiPointerUp$ services ?

I don't think we need separate services but if we can somehow provide and reuse Pan it would be nice

Unfortunately, I can't reuse pan service (or I dont understand how to do it).

PanService emits values on every *Move event but i also need first emission of *Down-event

projects/demo/src/modules/components/range/examples/2/index.less

waterplea · 2022-04-04T08:59:35Z

projects/demo/src/modules/components/range/examples/3/index.less

+@import 'taiga-ui-local';
+
+.range {
+    z-index: 1;


What is this for?

To overlap tick's label

when we click on tick's label, the click event fires on Range

waterplea · 2022-04-04T09:01:08Z

projects/demo/src/modules/components/range/examples/4/index.less

+    z-index: 1;
+
+    /* (Optionally) expand clickable area as you wish */
+    &:after {


I wonder if it makes sense to add a mixin for that

I am not sure we want support it :)
Range already has increased clickable area.

Manually expanding it is optional.
And this manually expanded area can be of any shape.

Moreover, creating of pseudo-element is rather simple)

projects/demo/src/modules/components/range/examples/import/insert-template.md

projects/kit/components/slider/slider-readonly.directive.ts

waterplea · 2022-04-04T10:19:07Z

projects/kit/components/range/range.style.less

+    top: @track-height / 2;
+    left: 0;
+    right: 0;
+    z-index: 1;


What is it for?

Thumb is filled by semi-transparent color during hover.
It will show what happens under it otherwise.

waterplea · 2022-04-04T10:24:07Z

projects/kit/components/range/range-change.directive.ts

+     * TODO replace with pointer events (when all supported browsers can handle them).
+     * Dont forget to use setPointerCapture instead of listening all documentRef events
+     */
+    private readonly pointerDown$ = merge(


I don't think we need separate services but if we can somehow provide and reuse Pan it would be nice

waterplea · 2022-04-04T10:26:25Z

projects/kit/components/range/range.component.ts

    ) {
        super(control, changeDetectorRef, documentRef, fromToTexts$);
    }

    get nativeFocusableElement(): TuiNativeFocusableElement | null {
-        if (this.computedDisabled || !this.dotLeft || !this.dotRight) {
+        const [sliderLeftRef, sliderRightRef] = this.slidersRefs;


Didn't know you could do that :) I would use const {first, last} :)

Array deconstructing is convenient here because we can choose naming.

Do you think that I should use const {first, last} instead ?

waterplea · 2022-04-04T10:33:23Z

projects/kit/components/range/range-change.directive.ts

+            .pipe(
+                tap(({clientX, target}) => {
+                    activeThumb = this.detectActiveThumb(clientX, target);
+                    elementRef.nativeElement.focus();


I wonder if we should just drop setNativeFocused completely and use native method everywhere, like here 🤔

I removed usage of setNativeFocused from kit/components/range-directory

projects/kit/components/range/range-change.directive.ts

projects/kit/components/slider/slider-readonly.directive.ts

well-done-bot · 2022-04-05T12:40:12Z

nsbarsukov self-assigned this Mar 25, 2022

nsbarsukov force-pushed the range-refactor branch 7 times, most recently from b17be23 to 13cfc63 Compare March 29, 2022 09:08

nsbarsukov changed the title ~~[WIP] refactor(kit): Range use Slider inside~~ refactor(kit): Range use Slider inside Mar 29, 2022

nsbarsukov marked this pull request as ready for review March 29, 2022 09:22

nsbarsukov requested review from waterplea, MarsiBarsi and vladimirpotekhin as code owners March 29, 2022 09:22

splincode approved these changes Mar 30, 2022

View reviewed changes

nsbarsukov changed the title ~~refactor(kit): Range use Slider inside~~ feat(kit): Range use Slider inside Mar 30, 2022

vladimirpotekhin reviewed Mar 31, 2022

View reviewed changes

nsbarsukov force-pushed the range-refactor branch from 5fbefed to daecb60 Compare April 4, 2022 09:21

waterplea approved these changes Apr 4, 2022

View reviewed changes

github-actions bot added the ready to merge label Apr 4, 2022

splincode reviewed Apr 4, 2022

View reviewed changes

projects/kit/components/slider/slider-readonly.directive.ts Outdated Show resolved Hide resolved

splincode reviewed Apr 4, 2022

View reviewed changes

projects/kit/components/slider/slider-readonly.directive.ts Outdated Show resolved Hide resolved

nsbarsukov force-pushed the range-refactor branch 3 times, most recently from dfac1cb to 42128a1 Compare April 5, 2022 07:08

nsbarsukov added 3 commits April 5, 2022 10:56

refactor(kit): Range use Slider inside

01d0812

fix(kit): Slider improve directive readonly

e77050e

chore(demo): Range extend example with ticks labels

8c8ad3f

nsbarsukov added 4 commits April 5, 2022 10:56

chore(demo): Range extend example with ticks labels

6355944

fix(kit): Range prevent cursor's blinking during dragging of the thumb

6a38240

chore(kit): Range review comments

9b97434

chore(kit): Slider replace Array by Set

6df9d6a

nsbarsukov force-pushed the range-refactor branch from 42128a1 to f52aecc Compare April 5, 2022 07:56

chore(kit): Range add not-chromium Edge support (16-18)

6824717

tinkoff-bot force-pushed the range-refactor branch from f52aecc to 6824717 Compare April 5, 2022 08:02

vladimirpotekhin approved these changes Apr 5, 2022

View reviewed changes

nsbarsukov merged commit 245c8d5 into main Apr 5, 2022

nsbarsukov deleted the range-refactor branch April 5, 2022 12:39

nsbarsukov linked an issue Apr 5, 2022 that may be closed by this pull request

🛠 - Sliders #256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kit): `Range` use `Slider` inside #1538

feat(kit): `Range` use `Slider` inside #1538

nsbarsukov commented Mar 25, 2022

lumberjack-bot bot commented Mar 25, 2022 •

edited

Loading

github-actions bot commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 25, 2022 •

edited

Loading

vladimirpotekhin Mar 31, 2022

nsbarsukov Apr 1, 2022

waterplea Apr 4, 2022

nsbarsukov Apr 4, 2022

waterplea Apr 4, 2022

nsbarsukov Apr 4, 2022

waterplea Apr 4, 2022

nsbarsukov Apr 4, 2022

nsbarsukov Apr 4, 2022

waterplea Apr 4, 2022

nsbarsukov Apr 4, 2022

waterplea Apr 4, 2022

waterplea Apr 4, 2022

nsbarsukov Apr 4, 2022

waterplea Apr 4, 2022

nsbarsukov Apr 4, 2022

well-done-bot bot commented Apr 5, 2022

feat(kit): Range use Slider inside #1538

feat(kit): Range use Slider inside #1538

Conversation

nsbarsukov commented Mar 25, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

lumberjack-bot bot commented Mar 25, 2022 • edited Loading

Pull request was closed ✔️

github-actions bot commented Mar 25, 2022 • edited Loading

codecov bot commented Mar 25, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

well-done-bot bot commented Apr 5, 2022

feat(kit): `Range` use `Slider` inside #1538

feat(kit): `Range` use `Slider` inside #1538

lumberjack-bot bot commented Mar 25, 2022 •

edited

Loading

github-actions bot commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 25, 2022 •

edited

Loading