feat(Input, Combobox, Autocomplete): add showCleanCross prop #3594

SchwJ · 2025-02-03T11:50:32Z

Проблема

Необходимо отрисовывать в инпуте крестик и очищать значение по клику на него.

Решение

Добавила пропы в Input и Combobox, отнаследовала проп в Autocomplete, написала скриншотные тесты и добавила примеры в документацию.

Ссылки

Задача
Гайд для Input с крестикой очистки
Тред в маттермосте с обсуждением дизайна

Чек-лист перед запросом ревью

Добавлены тесты на все изменения
✅ unit-тесты для логики
✅ скриншоты для верстки и кросс-браузерности
⬜ нерелевантно
Добавлена (обновлена) документация
✅ styleguidist для пропов и примеров использования компонентов
⬜ jsdoc для утилит и хелперов
⬜ комментарии для неочевидных мест в коде
⬜ прочие инструкции (README.md, contributing.md и др.)
⬜ нерелевантно
Изменения корректно типизированы
✅ без использования any (см. PR 2856)
⬜ нерелевантно
Прочее
✅ все тесты и линтеры на CI проходят
✅ в коде нет лишних изменений
✅ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)

This reverts commit 77dc112.

This reverts commit bba277c.

This reverts commit b5c6f99.

This reverts commit 01e3b31.

This reverts commit cbadb12.

SchwJ · 2025-02-07T04:32:19Z

packages/react-ui/components/Autocomplete/__docs__/Autocomplete.docs.stories.tsx

Пример в доку Autocomplete, так как он наследует свойство инпута showCleanCross

SchwJ · 2025-02-07T04:35:57Z

packages/react-ui/components/Autocomplete/__tests__/Autocomplete-test.tsx

Тест, проверяющий что в Autocomplete работает свойство showCleanCross. Достаточно selenium теста, так как визуально всё аналогично инпуту

SchwJ · 2025-02-07T05:12:52Z

packages/react-ui/components/ComboBox/__creevey__/ComboBox.creevey.mts

Creevey-тест, проверяющий Combobox:

появление крестика при вводе в инпут

очистка при клике на крестик

фокусировка по табу на крестике

SchwJ · 2025-02-07T05:14:29Z

packages/react-ui/components/ComboBox/__docs__/ComboBox.docs.stories.tsx

Пример в доку ComboBox

SchwJ · 2025-02-07T05:15:16Z

packages/react-ui/components/ComboBox/__stories__/Combobox.stories.tsx

История для теста Combobox

SchwJ · 2025-02-07T05:54:31Z

packages/react-ui/components/Input/Input.tsx

@@ -446,6 +453,20 @@ export class Input extends React.Component<InputProps, InputState> {

    return (
      <InputLayout
+        clearInput={() => {
+          if (this.props.onValueChange) {
+            this.props.onValueChange('');


для управляемого инпута и для оповещения при неуправляемом

SchwJ · 2025-02-07T05:55:12Z

packages/react-ui/components/Input/Input.tsx

+          if (this.props.onValueChange) {
+            this.props.onValueChange('');
+          }
+          if (this.input) {


для неуправляемого инпута

packages/react-ui/components/Input/Input.tsx

Funkicide · 2025-02-11T15:59:59Z

packages/react-ui/components/Input/__stories__/Input.stories.tsx

+      <Input showCleanCross size="small" value={valueSmall} onValueChange={setValueSmall} data-tid="small" />
+      <Input showCleanCross size="medium" value={valueMedium} onValueChange={setValueMedium} data-tid="medium" />
+      <Input showCleanCross size="large" value={valueLarge} onValueChange={setValueLarge} data-tid="large" />
+    </Gapped>


Кажется, можно оставить скриншоты только для размеров, а функциональность можно проверить через RTL.

Функциональность я тоже решила проверить скриншотами, так как:
а) нужно проверить обводку крестика по табу (и в этом же тесте удобно проверить что при последующем нажатии enter происходит очистка поля)
б) мне в целом не очень хочется плодить тесты, связанные с одним и тем же пропом.

Но твое предложение и мой пункт б) натолкнули меня на мысль, что стоит сделать один скриншотный тест, который в целом будет проверять работу пропа с крестиком: размеры, работу на по-разному управляемых контролах и фокус по табу.

Сейчас получается, что скриншот обводки по фокусу есть только для среднего размера, если поедет маленький или большой, то не увидим.

packages/react-ui/components/Input/Input.tsx

Funkicide

В целом всё хорошо, на мой взгляд. Единственное, кажется что много избыточных комментариев. Практически везде, где они есть, и так понятно, что происходит, исходя из контекста и названий переменных.

packages/react-ui/internal/themes/BasicLightTheme.ts

Funkicide

У меня замечаний, кроме тех двух нет. Можно Мишу звать.

SchwJ · 2025-02-13T11:20:16Z

После Роминого ревью:

убрала лишний фокус по совету Ромы
согласно апдейту со стороны дизайна: обновила иконку и обновила цвета (цвет иконки по умолчанию -- Secondary Text #757575 и по ховеру -- Text #222222, также по уточнению Андрея). Соответственно, обновились скрины.

mshatikhin

еще общее замечание:
в заголовке есть Input и Combobox, но фича добавить еще и в Autocomplete

и странная работа с фокусировкой крестика:

он фокусируется мышкой, рисуется обводка -- зачем?
обычно достаточно показывать рамку при фокусировке с клавиатуры

browser_SKA5YzKGo3.mp4

mshatikhin · 2025-02-17T08:20:16Z

packages/react-ui/components/CurrencyInput/CurrencyInput.tsx

@@ -24,7 +24,7 @@ export interface CurrencyInputProps
  extends Pick<AriaAttributes, 'aria-label'>,
    CommonProps,
    Override<
-      InputProps,
+      Omit<InputProps, 'showCleanCross'>,


хочется понять, почему в CurrencyInput не добавили крестик?

Решили с Андреем что он не нужен

mshatikhin · 2025-02-17T08:23:02Z

packages/react-ui/components/Input/Input.tsx

@@ -444,8 +451,23 @@ export class Input extends React.Component<InputProps, InputState> {
      <FocusControlWrapper onBlurWhenDisabled={this.resetFocus}>{this.getInput(inputProps)}</FocusControlWrapper>
    );

+    const clearInputAndFocusInput = () => {


давай перенесем этот метод в методы класса и сдеаем его private handleClearInput

Перенесла

mshatikhin · 2025-02-17T08:23:29Z

packages/react-ui/components/Input/Input.tsx

    return (
      <InputLayout
+        clearInput={clearInputAndFocusInput}


clearInput -- это callback, давай его назовем onClearInput

Переименовала

mshatikhin · 2025-02-17T08:24:24Z

packages/react-ui/components/Input/Input.tsx

    return (
      <InputLayout
+        clearInput={clearInputAndFocusInput}
+        showCleanCross={showCleanCross && this.state.needsShowCleanCross}
+        onCrossBlur={() => {


onCrossBlur лучше назвать по аналогии onCleanCrossBlur

Переименовала

mshatikhin · 2025-02-17T08:28:47Z

packages/react-ui/components/Input/Input.tsx

@@ -532,6 +558,7 @@ export class Input extends React.Component<InputProps, InputState> {
  private handleFocus = (event: React.FocusEvent<HTMLInputElement>) => {
    this.setState({
      focused: true,
+      needsShowCleanCross: !!this.input?.value,


правда ли это нормальная проверка?, если в value будет 0 числом, то крестик не покажется

Решила заменить на такое: !!this.input?.value || this.input?.value === '0'

а может ли случиться ситуация что в input.value будет число 0 ?
со строкой то все будет работать нормально

смог сделать такое в песочнице

mshatikhin · 2025-02-17T08:50:56Z

packages/react-ui/internal/CleanCrossIcon/CleanCrossIcon.tsx

+    }
+  };
+
+  const [focusedByTab, setFocusedByTab] = React.useState(false);


давай явно затипизируем useState -- это вообще лучше всегда делать, чтобы более сильно типизировать и ускорять компиляцию ts

Добавила

mshatikhin · 2025-02-17T08:52:14Z

packages/react-ui/internal/CleanCrossIcon/CleanCrossIcon.tsx

+  size?: SizeProp;
+}
+
+export const CleanCrossIcon: React.FunctionComponent<CleanCrossIconProps> = ({ size = 'small', style, ...rest }) => {


не вижу автотестов на CleanCrossIcon
лучше добавить, на react-testing-library

Добавила

mshatikhin · 2025-02-17T08:58:58Z

packages/react-ui/internal/CleanCrossIcon/CleanCrossIcon.tsx

+          rest.disabled && styles.rootDisabled(theme),
+          getSizeClassName(size),
+        )}
+        style={{ ...style }}


зачем тут { ...style } ?

можно просто style={style}

Убрала

mshatikhin · 2025-02-17T08:59:55Z

packages/react-ui/internal/CleanCrossIcon/CleanCrossIcon.tsx

+  };
+  const handleBlur = () => setFocusedByTab(false);
+
+  const tabIndex = rest.disabled ? -1 : 0;


tabIndex можно заинлайнить, нет практического смысла в этой переменной

Согласна

mshatikhin · 2025-02-17T09:06:10Z

packages/react-ui/internal/CleanCrossIcon/CrossIcon.tsx

@@ -0,0 +1,13 @@
+import React from 'react';
+import { XIcon16Light, XIcon20Light, XIcon24Regular } from '@skbkontur/icons/icons/XIcon';


иконки для контроллов нужно импортировать из папки /internal/icons2022 и там вроде даже уже есть папка XIcon

например вот как тут

Сделала

mshatikhin · 2025-02-17T13:11:48Z

странность, что два раза setFocusedByTab вызывается

sashachabin · 2025-02-18T09:28:50Z

packages/react-ui/internal/CleanCrossIcon/CleanCrossIcon.tsx

+  return (
+    <CommonWrapper {...rest}>
+      <button
+        tabIndex={tabIndex}


Для button нам не нужен tabindex, поскольку он интерактивный

Предлагаю указать tabindex="-1", чтобы кнпока фокусилась и не попадалась скринридеру.

Иначе переживаю, что мы очень усложним ввод с клавиатуры лишним табом — с учетом тех привычек, которые есть у пользователей

Screen.Recording.2025-02-18.at.15.09.23.mov

Я сделал небольшую демку с нативным input[type=search] с крестиком, где видно, что крестик не фокусится с клавиатуры.

И переадресовал вопрос ребятам из кружка доступности!

Кружок доступности предложил, а мы с Мишей и Егором заапрувили: от таба избавляемся и оставляем tabIndex={-1}

И type="button" :)

sashachabin · 2025-02-18T09:40:43Z

packages/react-ui/components/Input/Input.tsx

@@ -53,6 +53,9 @@ export interface InputProps
    Override<
      React.InputHTMLAttributes<HTMLInputElement>,
      {
+        /** Устанавливает иконку крестика, при нажатии на который инпут очищается. */
+        showCleanCross?: boolean;


А давай уберем слово show по аналогии с другими пропами. Например, у Input есть leftIcon и rightIcon без этого слова

sashachabin · 2025-02-18T09:41:57Z

packages/react-ui/components/Input/Input.tsx

@@ -149,10 +152,12 @@ export interface InputState {
  blinking: boolean;
  focused: boolean;
  needsPolyfillPlaceholder: boolean;
+  needsShowCleanCross: boolean;


Кажется, эта штука отвечает за показ, а не за needs. Я думаю, что правильней назвать это cleanCrossShowed

SchwJ added 11 commits February 3, 2025 16:24

chore(): create CleanCrossIcon and theme variables

cbadb12

chore(): add story

01e3b31

chore(): forbid prop in components

b5c6f99

chore(): add cross in InputLayout

bba277c

chore(): add value in state input

77dc112

Revert "chore(): add value in state input"

4a43990

This reverts commit 77dc112.

Revert "chore(): add cross in InputLayout"

e030589

This reverts commit bba277c.

Revert "chore(): forbid prop in components"

f158a1b

This reverts commit b5c6f99.

Revert "chore(): add story"

432489d

This reverts commit 01e3b31.

Revert "chore(): create CleanCrossIcon and theme variables"

d1dd5cf

This reverts commit cbadb12.

feat(Input): add clean cross prop in input

4cf6e49

SchwJ force-pushed the add-clean-cross-in-input-and-combobox branch from 8432121 to 4cf6e49 Compare February 5, 2025 13:41

feat(Input): add screenshots and refactor creevey test

c0bd781

SchwJ commented Feb 7, 2025

View reviewed changes

SchwJ changed the title ~~feat(Input): add clean cross prop~~ feat(Input, Combobox): add showCleanCross prop Feb 7, 2025

Funkicide self-requested a review February 10, 2025 15:25

Funkicide assigned Funkicide and unassigned Funkicide Feb 10, 2025

Funkicide reviewed Feb 11, 2025

View reviewed changes

packages/react-ui/components/Input/Input.tsx Outdated Show resolved Hide resolved

Funkicide reviewed Feb 11, 2025

View reviewed changes

packages/react-ui/components/Input/Input.tsx Outdated Show resolved Hide resolved

Funkicide reviewed Feb 11, 2025

View reviewed changes

packages/react-ui/components/Input/Input.tsx Show resolved Hide resolved

Funkicide reviewed Feb 11, 2025

View reviewed changes

packages/react-ui/components/Input/Input.tsx Outdated Show resolved Hide resolved

Funkicide reviewed Feb 11, 2025

View reviewed changes

packages/react-ui/internal/themes/BasicLightTheme.ts Outdated Show resolved Hide resolved

chore(): fix review comments

c939ec8

SchwJ requested a review from Funkicide February 13, 2025 05:57

Funkicide reviewed Feb 13, 2025

View reviewed changes

feat(Input): fix review comment, update cross icon and color

cd5e6a1

mshatikhin requested changes Feb 17, 2025

View reviewed changes

SchwJ changed the title ~~feat(Input, Combobox): add showCleanCross prop~~ feat(Input, Combobox, Autocomplete): add showCleanCross prop Feb 17, 2025

sashachabin reviewed Feb 18, 2025

View reviewed changes

		@@ -0,0 +1,13 @@
		import React from 'react';
		import { XIcon16Light, XIcon20Light, XIcon24Regular } from '@skbkontur/icons/icons/XIcon';

feat(Input, Combobox, Autocomplete): add showCleanCross prop #3594

Are you sure you want to change the base?

feat(Input, Combobox, Autocomplete): add showCleanCross prop #3594

Conversation

SchwJ commented Feb 3, 2025 • 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

SchwJ Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Funkicide left a comment

Choose a reason for hiding this comment

Funkicide left a comment

Choose a reason for hiding this comment

SchwJ commented Feb 13, 2025

mshatikhin left a 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

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

mshatikhin commented Feb 17, 2025

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

sashachabin Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

SchwJ commented Feb 3, 2025 •

edited

Loading

SchwJ Feb 12, 2025 •

edited

Loading

sashachabin Feb 18, 2025 •

edited

Loading