-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix(input-autocomplete): add error and success icons #1551
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 09ee6ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Собрана новая демка. |
Собрана новая демка. |
Собрана новая демка. |
Добавь |
Собрана новая демка. |
Собрана новая демка. |
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.
Тут был неправильный компонент как preview packages/input-autocomplete/src/component.screenshots.test.tsx
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.
Тогда более правильно будет оставить такой preview, а для этого вида создать отдельный тест
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.
Но по сути, там тестировался совсем другой компонент. Который никогда не упадет от изменений в этом
className={cn(styles.errorIcon, styles[`size-${size}`])} | ||
data-addon='error-icon' | ||
> | ||
<StatusBadge |
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.
Разве все это не должно быть частью FormControlMobile?
Сейчас получается сильное дублирование с тем, что есть в Input'е
Возможно стоит использовать его, либо сразу завести задачу на рефакторинг
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.
Делал по аналогии с BaseInput
. rightAddons
прокидываются сверху в FormControlMobile
.
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.
FormControlMobile это же вроде как универсальный ui компонент. В котором совсем нет логики, только отображение
Опишите проблему
В мобильной версии:
Шаги для воспроизведения
Задать проп success={true}
Не увидеть иконку успеха
Задать проп error
Не увидеть иконку ошибки
Ожидаемое поведение
При задании пропсов success и error должны отображаться иконки
Чек лист
Внешний вид
Дополнительная информация
Иконка ошибки отображается только в теме сайта