feat(Slider): introduce Slider component #1243

Arucard89 · 2024-01-15T10:37:33Z

Tickets: DATAUI-1616, UXRFC-199

gravity-ui-bot · 2024-01-15T10:39:29Z

Playwright Test Component is ready.

gravity-ui-bot · 2024-01-15T10:40:00Z

Preview is ready.

amje · 2024-01-30T13:34:49Z

@Arucard89 Do we want it to be part of v6?

Arucard89 · 2024-01-30T15:26:57Z

@Arucard89 Do we want it to be part of v6?

Actually I don't know. What's plan?
This component corresponds to the design. And I'm waiting for decision.
It would great if you can review it and we introduce Slider with v6.
Also it is fine if we add it in some minor version.

But I need Slider in uikit to create RangeInputPicker

amje · 2024-01-31T11:52:58Z

src/components/Slider/BaseSlider/BaseSlider.scss

@@ -0,0 +1,449 @@
+@use 'sass:math';
+@use '../../variables';
+@use '../sliderVariables';


Let's name the file variables.scss and use alias: @use '../variables' as slider-variables

amje · 2024-01-31T11:55:20Z

src/components/Slider/sliderTypes.d.ts

@@ -0,0 +1,50 @@
+import type {SliderProps as RcSliderProps, SliderRef as RcSliderRef} from 'rc-slider';


Please call the file just types.ts

amje · 2024-01-31T11:56:53Z

src/components/Slider/BaseSlider/BaseSlider.scss

+@use '../sliderVariables';
+@use '../../../../styles/mixins';
+
+$block: '.#{variables.$ns-new}base-slider';


Instead of base-slider I would call it ReactSlider to show that is external component

Consider, please, this variant:
rc-slider has prop prefixCls for this purpose. If it is undefined then it add default classname and classname prefix 'rc-slider' to all components inside it. Using it we save accordance between component name and classname. And also show that component is external.
Whole classname of BaseComponent will look like this:
'rc-slider g-base-slider g-base-slider_size_s rc-slider-horizontal'

1837e16

src/components/Slider/README.md

amje · 2024-01-31T12:35:37Z

src/components/Slider/Slider.scss

+
+    &__error {
+        &_size_xl {
+            @include mixins.text-body-2;


We have one size in inputs, slider should follow the same rules

You mean that error always should have text-body-1? But then it will not correspond to design. Is it OK?

Yes, it should corelate with other control's errors

amje · 2024-01-31T12:43:20Z

src/components/Slider/Slider.tsx

+        infoPointCount = 0,
+        availableValues,
+        withTooltip = false,
+        error,


error -> errorMessage + validationState: 'invalid', check the TextInput or TextArea component for reference

amje · 2024-01-31T12:43:46Z

src/components/Slider/Slider.tsx

+        min = 0,
+        max = 100,
+        step = 1,
+        infoPointCount = 0,


Maybe name this prop marks or marksCount?

Yes, your names are good(especially marksCount) and I like them but I took this name from RangeInputPicker. I thought that saving of the name would made transition to new component easier.
What do you think?

It's a great opportunity to change the API

amje · 2024-01-31T12:44:12Z

src/components/Slider/Slider.tsx

+        withTooltip = false,
+        error,
+        disabled = false,
+        keyboard = true,


Component must always be accessible with keyboard, this prop not needed

amje · 2024-01-31T12:49:15Z

src/components/Slider/Slider.tsx

+) {
+    const baseSliderRef = React.useRef<BaseSliderRefType>(null);
+
+    React.useImperativeHandle(ref, () => ({


Let the ref be normal ref to HTMLElement, and for this case use apiRef dedicated prop

amje · 2024-01-31T12:54:56Z

src/components/Slider/index.ts

@@ -0,0 +1 @@
+export * from './Slider';


Let's be explicit here, we should export component and alose types we assume are public

Arucard89 · 2024-02-05T13:10:25Z

@amje , hi. I've fixed all comments. Check them, please

amje · 2024-02-07T14:50:21Z

src/components/Slider/Slider.scss

+        }
+    }
+
+    &__error {


There are rules for &__error at the top of the file

amje · 2024-02-07T14:58:50Z

src/components/Slider/types.ts

+    onChange?: (value: ValueType) => void;
+    /** Fires when ontouchend or onmouseup is triggered. Provides change event as an callback's argument */
+    onChangeComplete?: (value: ValueType) => void;


If handlers receive only the value, then we call them onUpdate*, not onChange

amje · 2024-02-07T15:02:31Z

src/components/Slider/types.ts

+    /** Indicates that the user cannot interact with the control */
+    disabled?: boolean;
+    /** Text of an error to show */
+    errorText?: string;


Let's name this errorMessage, like in TextInput and TextArea

amje · 2024-02-07T15:06:51Z

src/components/Slider/index.ts

@@ -0,0 +1,2 @@
+export * from './Slider';
+export type {SliderSize, SliderValue, SliderProps, BaseSliderRefType} from './types';


Why BaseSliderRefType need to be exported?

Slider has prop:

apiRef?: React.RefObject<BaseSliderRefType>;

So we need to export this type to allow user write variable type correctly.

amje · 2024-02-07T15:12:59Z

src/components/Slider/__stories__/Slider.stories.tsx

+export default {
+    title: 'Components/Inputs/Slider',
+    component: Slider,
+    tags: ['autodocs'],


We don't use autodocs. Instead we use mdx for docs, check Button's stories for a reference

fixed: c433487

…)-Slider

Arucard89 · 2024-02-09T08:11:41Z

@amje hi. I fixed all comments. Could you review it?

Arucard89 marked this pull request as ready for review January 26, 2024 06:57

Arucard89 requested review from amje, ValeraS and korvin89 as code owners January 26, 2024 06:57

Arucard89 changed the title ~~feat: introduce Slider component~~ feat(Slider): introduce Slider component Jan 26, 2024

amje requested changes Jan 31, 2024

View reviewed changes

Arucard89 force-pushed the DATAUI-1616(UXRFC-199)-Slider branch 4 times, most recently from dd079a5 to cde5020 Compare February 4, 2024 21:14

Arucard89 force-pushed the DATAUI-1616(UXRFC-199)-Slider branch 3 times, most recently from 7c3dcb4 to a82983d Compare February 7, 2024 12:05

amje requested changes Feb 7, 2024

View reviewed changes

Arucard89 added 11 commits February 8, 2024 01:00

feat: rc-slider to deps

24a1e7e

feat: create BaseSlider component according to figma styles

0d707c5

feat: add Slider component based on BaseSlider

c8e298e

feat: add readme and stories

de6f17c

feat: move pushable property to base slider defaults

b9840bb

chore: rename css variables

323e440

feat: add tooltip

74cd03a

feat: make tooltip non-interactive

bbb4d7f

feat: add dots to slider

1524141

feat: storibook - default template

8fb5e9c

fix: slider type and tooltip property

20ed639

Arucard89 added 16 commits February 8, 2024 01:00

feat: add ref and apiRef

44986bd

feat: add invalidation state and error text

dbd3129

chore: styleModifiers->stateModifiers

c763f75

feat: split slider active and focus states

e63593c

fix: handle grabbing

1f8bf77

fix: mde apiref optional

631d013

feat: add types description

255fa69

feat: add readme

b5038e6

fix: change tooltip text color

84c2c0d

fix: lint errors for css

4a2a1d1

fix: rename blockNew->block

c264d2e

fix: export -> export type

3b2a8ca

fix: ns-new -> ns

08a4d99

fix: after rebase

c197f05

feat: add rtl to tooltip

328889d

feat: add rtl support

7671677

Arucard89 force-pushed the DATAUI-1616(UXRFC-199)-Slider branch 2 times, most recently from d9f9a6b to 7671677 Compare February 8, 2024 08:50

Arucard89 added 6 commits February 9, 2024 10:31

feat: replace autodocs

c433487

Merge remote-tracking branch 'origin/main' into DATAUI-1616(UXRFC-199…

1450d56

…)-Slider

chore: rename errorText->errorMessage

b266724

chore: rename onChange->onUpdate

879a8be

fix: move error styles in one block

21803ad

chore: fix text of comment

44f2c6e

Merge branch 'main' into DATAUI-1616(UXRFC-199)-Slider

b5935e9

korvin89 approved these changes Feb 19, 2024

View reviewed changes

korvin89 merged commit a1bf754 into main Feb 19, 2024
4 checks passed

korvin89 deleted the DATAUI-1616(UXRFC-199)-Slider branch February 19, 2024 17:51

gravity-ui-bot mentioned this pull request Feb 19, 2024

chore(main): release 6.1.0 #1316

Merged

		@@ -0,0 +1,50 @@
		import type {SliderProps as RcSliderProps, SliderRef as RcSliderRef} from 'rc-slider';

		@@ -0,0 +1,2 @@
		export * from './Slider';
		export type {SliderSize, SliderValue, SliderProps, BaseSliderRefType} from './types';

feat(Slider): introduce Slider component #1243

feat(Slider): introduce Slider component #1243

Conversation

Arucard89 commented Jan 15, 2024

gravity-ui-bot commented Jan 15, 2024

gravity-ui-bot commented Jan 15, 2024

amje commented Jan 30, 2024

Arucard89 commented Jan 30, 2024

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

Arucard89 Jan 31, 2024 • edited Loading

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

Arucard89 Jan 31, 2024 • edited Loading

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

Arucard89 commented Feb 5, 2024

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

Arucard89 commented Feb 9, 2024

Arucard89 Jan 31, 2024 •

edited

Loading

Arucard89 Jan 31, 2024 •

edited

Loading