-
Notifications
You must be signed in to change notification settings - Fork 132
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 showClearIcon 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.
У меня замечаний, кроме тех двух нет. Можно Мишу звать.
После Роминого ревью:
|
@@ -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
Outdated
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 после инициализации
# Conflicts: # packages/react-ui/internal/InputLikeText/InputLikeText.tsx # packages/react-ui/internal/themes/BasicDarkTheme.ts # packages/react-ui/internal/themes/BasicLightTheme.ts
//#region ClearCrossIcon | ||
public static clearCrossIconColor = 'rgba(255, 255, 255, 0.32)'; | ||
public static get clearCrossIconHoverColor() { | ||
return this.inputBorderColorFocus; |
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.
почему тут ссылка на цвет inputBorderColorFocus ?
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.
это может привести к тому что крестик станет цветным в темной теме
@@ -44,6 +44,14 @@ export const DarkTheme5_0 = createTheme({ | |||
//#region CloseIcon, CloseButtonIcon | |||
public static closeBtnIconColor = 'rgba(255, 255, 255, 0.32)'; | |||
//#endregion CloseIcon, CloseButtonIcon | |||
|
|||
//#region ClearCrossIcon | |||
public static clearCrossIconColor = 'rgba(255, 255, 255, 0.32)'; |
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.
тут кажется не правильно, откуда циферка 0.32 взялась?
по макетам должно быть 0.54
https://www.figma.com/design/tEz0VI62iYNf7xpnouiU0f/%E2%9A%A1-Kontur-UI-%5BDark%5D?node-id=1-41257&t=WSkBvAcSmdZtrB8b-0
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.
Поняла, что у нас толком нигде не зафиксированы все возможные цвета крестика. Решила уточнить у Саши Кряжева, какие цвета крестика должны быть:
- в светлой теме при обычном состоянии? (Андрей когда-то говорил 'rgba(117, 117, 117, 1)' = '#757575')
- в светлой теме при наведении на крестик? (Андрей когда-то говорил 'rgba(34, 34, 34, 1)' = '#222222')
- в темной теме при обычном состоянии? (вроде 'rgba(255, 255, 255, 0.54)')
- в темной теме при наведении на крестик? (вроде 'rgba(255, 255, 255, 0.865)')
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.
Определили с Сашей нужные цвета:
В светлой теме:
rgba(0, 0, 0, 0.54) — обычное состояние
rgba(0, 0, 0, 0.87) — при наведении
В темной теме:
rgba(255, 255, 255, 0.54) — обычное состояние
rgba(255, 255, 255, 0.865) — при наведении
}; | ||
render(<ControlledAutocomplete />); | ||
const autocomplete = screen.getByRole('textbox'); | ||
expect(screen.queryByTestId(InputDataTids.clearCross)).toBeNull(); |
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.
|
||
export const ClearCrossIcon: React.FunctionComponent<ClearCrossIconProps> = ({ size = 'small', style, ...rest }) => { | ||
const _theme = React.useContext(ThemeContext); | ||
const theme = ThemeFactory.create( |
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, написала скриншотные тесты и добавила примеры в документацию.
Ссылки
Задача
Гайд для Input с крестикой очистки
Тред в маттермосте с обсуждением дизайна
Чек-лист перед запросом ревью
Добавлены тесты на все изменения
✅ unit-тесты для логики
✅ скриншоты для верстки и кросс-браузерности
⬜ нерелевантно
Добавлена (обновлена) документация
✅ styleguidist для пропов и примеров использования компонентов
⬜ jsdoc для утилит и хелперов
⬜ комментарии для неочевидных мест в коде
⬜ прочие инструкции (
README.md
,contributing.md
и др.)⬜ нерелевантно
Изменения корректно типизированы
✅ без использования
any
(см. PR2856
)⬜ нерелевантно
Прочее
✅ все тесты и линтеры на CI проходят
✅ в коде нет лишних изменений
✅ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)