feat(NumberInput): add new component #1826

DaryaLari · 2024-08-28T09:25:09Z

No description provided.

gravity-ui-bot · 2024-08-28T09:27:12Z

Preview is ready.

gravity-ui-bot · 2024-08-28T09:27:54Z

Visual Tests Report is ready.

DaryaLari · 2024-09-16T16:45:53Z

@korvin89 @amje
Hope that for now I have fixed all visible flaws... Could you check the changes, please?

amje · 2024-09-26T19:50:36Z

src/components/lab/NumberInput/__stories__/NumberInput.stories.tsx

+    onKeyPress: () => {},
+};
+
+const DefaultTemplate: StoryFn<NumberInputProps> = (args) => {


Let's use newer format of stories (CSF3), see Button as a reference. Right now controls are unusable. The idea is that one single story should show particular prop behavior and be altered with controls. For example, Size - variants of sizes, View - variants of views, Error - variants of how error can be displayed, Step - how step works, Min/Max - how minimum and maximum works and so on.

amje · 2024-09-26T19:51:35Z

src/components/lab/NumberInput/__tests__/NumberInput.visual.test.tsx

+        await expectScreenshot();
+    });
+
+    test('render story: <Showcase>', async ({mount, expectScreenshot}) => {


Let's keep only default story screenshot for now, current stories are not suitable for such kind of tests

amje · 2024-09-26T19:56:22Z

src/components/lab/NumberInput/NumberInput.tsx

+                onInput: handleInput,
+                ...props.controlProps,
+                onWheel: allowMouseWheel && active ? handleWheel : undefined,
+                role: 'spinbutton',


Not sure if it's correct. It's an input for numeric text, not spinbutton.

Native input type number has spinbutton role

Ok, let's keep it.

amje · 2024-09-26T19:57:39Z

src/components/controls/TextInput/TextInput.tsx

@@ -94,7 +104,7 @@ export const TextInput = React.forwardRef<HTMLSpanElement, TextInputProps>(
            note,
            onUpdate,
            onChange,
-        } = props;
+        } = propsWithDefaults;


Why we need this? Defaults eventually will be aplied anyway.

Because some of these TextInput defaults (which were moved to a separate object) are also required in NumberInput

Can we use it in React way then? TextInput.defaultProps = {...}

defaultProps for function components is deprecated in React 19. https://react.dev/blog/2024/04/25/react-19-upgrade-guide#removed-deprecated-react-apis

If you'd help me to deal with fc typings with defaultProps^ then I'll change it to react way
I would really appreciate you help

From default props only size and view are needed, right? Let's set default only for them ('m', and 'normal') in NumberInput.

@ValeraS
Also disabled is used in NumberInput directly.
But, wouldn't it be better to store default props in one object to be sure that they wouldn't differ in different controls one day? And what if we'll need to use more default props from TextInput in NumberInput itself?

Ok, I've checked that other controls define default values of these props themselves, so I reverted these changes in TextInput

amje · 2024-09-26T20:00:30Z

src/components/lab/NumberInput/NumberInput.tsx

+        value,
+        defaultValue,
+        shiftMultiplier,
+    } = getInternalVariables({


getInternalState seems to be more descriptive name

amje · 2024-09-26T20:10:48Z

src/components/lab/NumberInput/NumberInput.tsx

+        onBlur?.(e);
+    };
+
+    const handleChange: React.ChangeEventHandler<HTMLInputElement> = (e) => {


We can only use onUpdate for updating the value. onChange handler is redundant

amje · 2024-09-26T20:13:58Z

src/components/lab/NumberInput/NumericArrows/NumericArrows.scss

+    );
+
+    --_--border-width: var(--g-text-input-border-width, 1px);
+    border-style: solid;


Seems like these styles are duplicated

Which styles and where is their duplicate is located?

border styles between NumericArrows.scss and NumberInput.scss

amje · 2024-09-26T20:14:45Z

src/components/lab/NumberInput/NumericArrows/NumericArrows.scss

+@use '../../../../../styles/mixins';
+@use '../../../variables';
+@use '../../../controls/mixins.scss' as control-mixins;
+@use '../../../controls/variables.scss' as control-variables;


We don't use any of these in the file

amje · 2024-09-26T20:21:08Z

src/components/lab/NumberInput/index.ts

+export {getNumericInputValidator} from './utils';
+export type {GetNumericInputValidatorParams} from './utils';


Aren't these internal helpers? Why we export them?

No, they are not used in component. This utility function is for external use only. NumberInput itself doesn't show any validation errors

Then why keep this code? We cannot maintain it properly without specific usecases

amje · 2024-09-26T20:23:11Z

src/components/lab/NumberInput/utils.ts

+    return clampedValue;
+}
+
+export function warnAboutInvalidProps({


This will span a lot. What's the benefit of this? Component won't allow invalid value anyway.

It will spam only with invalid props passed.
I think it would be better to notify programmer about invalid component usage6 but not just fix props silently

Ok then it should use warnOnce function from utils in format of [NumberInput] <message> instead of console.warn, see usages across the codebase

…es to divisible numbers

…orwardRef&comment in N-Arrows

…trols

DaryaLari · 2024-10-07T11:33:41Z

@ValeraS @amje @korvin89
Hopefully I've fixed all previous comments and I'm waiting for your review again

amje · 2024-10-15T17:12:01Z

src/components/lab/NumberInput/NumberInput.scss

+    }
+
+    &_view_normal {
+        --_-g-number-input-numeric-arrows-border-color: var(--g-color-line-generic);


Suggested change

--_-g-number-input-numeric-arrows-border-color: var(--g-color-line-generic);

--_--arrows-border-color: var(--g-color-line-generic);

It's a private var so no need to specify full name

amje · 2024-10-15T17:15:57Z

src/components/lab/NumberInput/NumberInput.scss

+        --_--border-color-hover: var(--g-color-line-generic-hover);
+        --_--border-color-active: var(--g-color-line-generic-active);


What these variables for? They duplicate TextInput styles

amje · 2024-10-15T17:16:11Z

src/components/lab/NumberInput/NumberInput.scss

+        --_-g-number-input-numeric-arrows-border-color: transparent;
+    }
+
+    &__numeric-arrows {


Suggested change

&__numeric-arrows {

&__arrows {

Let's be compact here

amje · 2024-10-15T17:19:01Z

src/components/lab/NumberInput/NumberInput.tsx

+    /** An optional element displayed under the lower right corner of the control and sharing the place with the error container */
+    note?: React.ReactNode;
+    /**Describes the validation state */
+    validationState?: 'invalid' | undefined;


This prop already in the BaseInputControlProps, no need to specify it again

amje · 2024-10-15T17:24:15Z

src/components/lab/NumberInput/NumberInput.tsx

+    /** Enables changing value by scrolling mousewheel on with cursor on the input
+     * @default  false
+     */
+    allowMouseWheel?: boolean;


We've decided to remove the wheel behaviour completely. It turned out that currently it brings a lot of bugs and inconsistency between mouse and touchpad devices. Maybe we reimplement it in the future.

amje · 2024-10-15T17:30:40Z

src/components/lab/NumberInput/NumberInput.tsx

+                onInput: handleInput,
+                ...props.controlProps,
+                onWheel: allowMouseWheel && active ? handleWheel : undefined,
+                role: 'spinbutton',


Ok, let's keep it.

amje · 2024-10-15T17:31:27Z

src/components/lab/NumberInput/NumberInput.tsx

+                ...props.controlProps,
+                onWheel: allowMouseWheel && active ? handleWheel : undefined,
+                role: 'spinbutton',
+                inputMode: 'numeric',


when allowDecimal = true, we should pass decimal here to have correct keyborad on mobile devices

amje · 2024-10-15T17:32:04Z

src/components/lab/NumberInput/NumericArrows/NumericArrows.scss

+$block: '.#{variables.$ns}numeric-arrows';
+
+#{$block} {
+    --_--numeric-arrows-border-color: var(


Suggested change

--_--numeric-arrows-border-color: var(

--_--arrows-border-color: var(

amje · 2024-10-15T17:33:47Z

src/components/lab/NumberInput/NumericArrows/NumericArrows.tsx

+        view: 'flat-secondary',
+        disabled,
+        tabIndex: -1,
+        width: 'max',


Let's hide the buttons from screenreaders via aria-hidden="true"

amje · 2024-10-15T17:38:41Z

src/components/lab/NumberInput/index.ts

+export {getNumericInputValidator} from './utils';
+export type {GetNumericInputValidatorParams} from './utils';


Then why keep this code? We cannot maintain it properly without specific usecases

amje · 2024-10-15T17:45:53Z

Extra zero digits not deleted on blur:

2024-10-15.20.41.16.mov

Typing dot when allowDecimal = false deletes one digit at time, should do nothing:

2024-10-15.20.41.37.mov

Leading dot not deleted on blur:

2024-10-15.20.42.29.mov

DaryaLari changed the title ~~NumberInput~~ feat(NumberInput): add new component Aug 28, 2024

DaryaLari force-pushed the number-input branch 4 times, most recently from 68cd582 to f529fed Compare August 29, 2024 08:29

DaryaLari marked this pull request as ready for review August 29, 2024 08:44

DaryaLari requested review from korvin89, amje and ValeraS as code owners August 29, 2024 08:44

DaryaLari force-pushed the number-input branch from f529fed to af30ed7 Compare September 13, 2024 13:44

DaryaLari marked this pull request as draft September 13, 2024 13:52

DaryaLari force-pushed the number-input branch from 517519a to c7425bd Compare September 13, 2024 14:01

DaryaLari marked this pull request as ready for review September 15, 2024 07:44

DaryaLari force-pushed the number-input branch from e86d5d6 to fc7fce7 Compare September 16, 2024 16:39

DaryaLari force-pushed the number-input branch 3 times, most recently from be77a52 to 29bfffd Compare September 19, 2024 16:34

DaryaLari force-pushed the number-input branch 4 times, most recently from cd9c3bd to cc9165b Compare September 26, 2024 10:12

amje requested changes Sep 26, 2024

View reviewed changes

DaryaLari force-pushed the number-input branch 3 times, most recently from b7b173b to faf3eaa Compare September 30, 2024 07:38

DaryaLari requested a review from amje September 30, 2024 07:43

DaryaLari force-pushed the number-input branch from 255d426 to feac75f Compare September 30, 2024 16:41

DaryaLari added 19 commits October 4, 2024 16:14

fix(NumberInput): story item value update

94fd8f9

fix(NumberInput): fix after rebase

4d34067

fix(NumberInput): arrows width

f368d8b

fix(NumberInput): inserting invalid string to the middle of number

f9d33ef

chore(NumberInput): change value type to number and add clamping valu…

3502291

…es to divisible numbers

test: update snapshots

07db0ef

chore(NumberInput): move component to lab

bb4d866

fix(NumberInput): review fixes

96c899c

docs(NumberInput): fix types in readme

cc4d7fb

docs(NumberInput): add description of behaviour props to readme

63f2e8e

fix(NumberInput): more review fixes

68a529b

fix(NumberInput): duplicated arrows border style

c030749

fix(NumberInput): add e.preventDefault, rename showControls, remove f…

69c0a1d

…orwardRef&comment in N-Arrows

fix(NumberInput): fix clapting to step divisibles

152bdfe

fix(NumberInput): remove local step state

1ff5952

chore(NumberInput): handle home/end btns press

165316a

test(NumberInput): add tests on HOME/END press

f4f5297

fix(NumberInput): remove exporting default props from TextInput

bda3863

fix(NumberInput): hide description&default columns from storybook con…

c4daf69

…trols

DaryaLari force-pushed the number-input branch 2 times, most recently from 2174b81 to f54c958 Compare October 4, 2024 13:17

feat(NumberInput): support uncontrolled usage type

ddfd727

DaryaLari force-pushed the number-input branch from f54c958 to ddfd727 Compare October 4, 2024 13:25

DaryaLari added 2 commits October 4, 2024 17:00

chore(NumberInput): change input type in default story to uncontrolled

b53f6bd

chore(NumberInput): change all other stiries' inputs to uncontrolled

7a98458

DaryaLari marked this pull request as draft October 7, 2024 08:58

DaryaLari marked this pull request as ready for review October 7, 2024 08:59

amje requested changes Oct 15, 2024

View reviewed changes

		export {getNumericInputValidator} from './utils';
		export type {GetNumericInputValidatorParams} from './utils';

	--_-g-number-input-numeric-arrows-border-color: var(--g-color-line-generic);
	--_--arrows-border-color: var(--g-color-line-generic);

		--_--border-color-hover: var(--g-color-line-generic-hover);
		--_--border-color-active: var(--g-color-line-generic-active);

	--_--numeric-arrows-border-color: var(
	--_--arrows-border-color: var(

feat(NumberInput): add new component #1826

Are you sure you want to change the base?

feat(NumberInput): add new component #1826

Conversation

DaryaLari commented Aug 28, 2024

gravity-ui-bot commented Aug 28, 2024

gravity-ui-bot commented Aug 28, 2024

DaryaLari commented Sep 16, 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

Choose a reason for hiding this comment

DaryaLari Oct 3, 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

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

DaryaLari commented Oct 7, 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

amje commented Oct 15, 2024

DaryaLari Oct 3, 2024 •

edited

Loading