-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Input, Combobox, Autocomplete): add showCleanCross prop #3594
base: next
Are you sure you want to change the base?
Conversation
8432121
to
4cf6e49
Compare
packages/react-ui/components/Autocomplete/__docs__/Autocomplete.docs.stories.tsx
Outdated
Show resolved
Hide resolved
packages/react-ui/components/Autocomplete/__tests__/Autocomplete-test.tsx
Show resolved
Hide resolved
packages/react-ui/components/ComboBox/__creevey__/ComboBox.creevey.mts
Outdated
Show resolved
Hide resolved
packages/react-ui/components/ComboBox/__docs__/ComboBox.docs.stories.tsx
Outdated
Show resolved
Hide resolved
packages/react-ui/components/ComboBox/__stories__/Combobox.stories.tsx
Outdated
Show resolved
Hide resolved
packages/react-ui/components/Input/__stories__/Input.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В целом всё хорошо, на мой взгляд. Единственное, кажется что много избыточных комментариев. Практически везде, где они есть, и так понятно, что происходит, исходя из контекста и названий переменных.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У меня замечаний, кроме тех двух нет. Можно Мишу звать.
После Роминого ревью:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
еще общее замечание:
в заголовке есть Input и Combobox, но фича добавить еще и в Autocomplete
и странная работа с фокусировкой крестика:
- он фокусируется мышкой, рисуется обводка -- зачем?
обычно достаточно показывать рамку при фокусировке с клавиатуры
browser_SKA5YzKGo3.mp4
@@ -53,6 +53,9 @@ export interface InputProps | |||
Override< | |||
React.InputHTMLAttributes<HTMLInputElement>, | |||
{ | |||
/** Устанавливает иконку крестика, при нажатии на который инпут очищается. */ | |||
showCleanCross?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А давай уберем слово show
по аналогии с другими пропами. Например, у Input
есть leftIcon
и rightIcon
без этого слова
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Сейчас немного сменилась идея показа крестика. Он может показываться
- всегда при непустом инпуте "always"
- при фокусе на непустом инпуте "onFocus"
- никогда "never"
В таком случае мне кажется нецелесообразным менять название, так как "always", "onFocus" и "never" являются наречиями, а наречия в свою очередь всегда зависят от глагола
Обсудили название, решили заменить |
Название коммита выше неправильное. Переименовала showCleanCross в showClearIcon |
packages/react-ui/internal/CustomComboBox/CustomComboBoxReducer.tsx
Outdated
Show resolved
Hide resolved
packages/react-ui/components/Autocomplete/__tests__/Autocomplete-test.tsx
Show resolved
Hide resolved
@@ -26,6 +27,7 @@ import { PolyfillPlaceholder } from './InputLayout/PolyfillPlaceholder'; | |||
export const inputTypes = ['password', 'text', 'number', 'tel', 'search', 'time', 'date', 'url', 'email'] as const; | |||
|
|||
export type InputAlign = 'left' | 'center' | 'right'; | |||
export type ShowClearIcon = 'always' | 'onFocus' | 'never'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не могу придумать лучше чем always,
но always может вводить в заблуждение что крестик вообще всегда будет показываться
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Есть такие идеи:
- alwaysWhenNotEmpty (тогда 'onFocus' -> 'onFocusWhenNotEmpty')
- alwaysIfFilled (тогда 'onFocus' -> 'onFocusIfFilled')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sashachabin Призываем тебя!
Может есть идеи?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Правильно будет:
showClearIcon="whenFilled"
showClearIcon="whenFilledOnFocus"
Но это слишком длинно, поэтому предлагаю:
showClearIcon="filled"
showClearIcon="filledOnFocus"
илиshowClearIcon="filledFocus"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Подглядеть некуда, его либо нет, либо это boolean:
- NextUI (isClearable: bool)
- Ant.design (allowClear: bool)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
После обсуждения решили:
auto
— показывается по ховеру или фокусуalways
— показывается всегдаnever
— никогда не показывать (по умолчанию)
@@ -436,6 +445,17 @@ export class ComboBoxView<T> extends React.Component<ComboBoxViewProps<T>, Combo | |||
return <LoadingIcon size={size} />; | |||
} | |||
|
|||
if (this.getProps().showClearIcon !== 'never' && this.state.clearCrossShowed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.getProps().showClearIcon !== 'never' -- это условие не нужно поидее,
достаточно смотреть на this.state.clearCrossShowed и обновлять state при необходимости (например в componentDidUpdate)
@@ -436,6 +445,17 @@ export class ComboBoxView<T> extends React.Component<ComboBoxViewProps<T>, Combo | |||
return <LoadingIcon size={size} />; | |||
} | |||
|
|||
if (this.getProps().showClearIcon !== 'never' && this.state.clearCrossShowed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не нашел кода который бы устанавливал this.state.clearCrossShowed после инициализации
Проблема
Необходимо отрисовывать в инпуте крестик и очищать значение по клику на него.
Решение
Добавила пропы в Input и Combobox, отнаследовала проп в Autocomplete, написала скриншотные тесты и добавила примеры в документацию.
Ссылки
Задача
Гайд для Input с крестикой очистки
Тред в маттермосте с обсуждением дизайна
Чек-лист перед запросом ревью
Добавлены тесты на все изменения
✅ unit-тесты для логики
✅ скриншоты для верстки и кросс-браузерности
⬜ нерелевантно
Добавлена (обновлена) документация
✅ styleguidist для пропов и примеров использования компонентов
⬜ jsdoc для утилит и хелперов
⬜ комментарии для неочевидных мест в коде
⬜ прочие инструкции (
README.md
,contributing.md
и др.)⬜ нерелевантно
Изменения корректно типизированы
✅ без использования
any
(см. PR2856
)⬜ нерелевантно
Прочее
✅ все тесты и линтеры на CI проходят
✅ в коде нет лишних изменений
✅ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)