Skip to content
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(Clickable): add new component #3267

Open
wants to merge 38 commits into
base: next
Choose a base branch
from
Open

feat(Clickable): add new component #3267

wants to merge 38 commits into from

Conversation

JackUait
Copy link
Contributor

@JackUait JackUait commented Sep 14, 2023

Проблема

Пользователям нужен компонент, который может выглядеть как кнопка или как ссылка, при этом под капотом у которого может быть любой тег: button, a и даже div, для тех случаев когда нужен внешний вид, но не нужна семантика. Например, когда сверху компонента есть <button> и внутрь нужно вложить нашу кнопку — в таком случае пригодится просто представление компонента без семантики и обработки событий, которую организуют <button> и <a>.

Решение

Создал компонент <Clickable>, который представляет из себя единый интерфейс доступа к внешнему виду и семантике кнопки и ссылки. Компонент основан на обсуждении из #2598 и задаче IF-959, а также ещё некоторых незадокументированных обсуждениях.

Идея:
Я добавил два новых пропа: as и view. Они призваны отделить внешнее представление компонента от его семантики. Проп as позволяет задавать корневой тег, а проп view задаёт внешний вид контрола.

Изменения:
Помимо нового компонента и пропов я внёс несколько изменений в API, которые призваны сделать его более удобным для пользователей, а также закрыл несколько параллельных задач

  1. Появился новый про rightIcon (IF-1450);
  2. Компонент не поддерживает DEFAULT_THEME и IE11. Я отказался от их поддержки, в пользу простоты кода, с учётом того, что в скором будущем мы планируем отказаться от их поддержки;
  3. Проп arrow теперь принимает 'left' | 'right', вместо boolean | 'left';
  4. У контрола расширился use в связи с тем, что он теперь хранит use'ы, которые ему достались от Button и Link;
  5. В <Clickable> нет скриншота со сбросом (reset) стилей, так как стили стали сбрасываться через all: unset, который не сбрасывает те стили, которые проверяются в скриншоте. Настолько сильно сбрасывать стили мне показалось избыточным, поэтому я не стал реализовывать эту логику в новом компоненте;
  6. Я убрал селектор по :enabled из стилей <ClickableButton>, так как только кнопка поддерживает этот псевдокласс. Он заменён на селектор по aria-disabled, там где это необходимо;
  7. Упростил логику и переписал <ComponentTable> в силу того, что он не умел работать с функциональными компонентами. Я удалил часть логики из <ComponentTable>, но она не использовалась ни в одном из тестируемых компонентов. После этого пришлось обновить несколько скриншотов, но на самих скриншотах ничего не изменилось, кроме того, что в некоторых местах пропали кавычки от <ComponentTable>, изменился порядок пропов, если в компонент передаётся children, а также стали выводиться все пропы, которые передаются в children.

Исправления/улучшения:

  1. На корень больше не прокидывается атрибут disabled (IF-1661). Вместо него прокидывается атрибут aria-disabled. Это позволяет фокусироваться на кнопке и сообщать пользователям скрин-ридеров, что она отключена. На замену HTML-состоянию disabled в стилях для состояния контрола disabled пришёл стиль pointer-events: none;
  2. Добавлена поддержка всех атрибутов кнопки и ссылки, включая aria-аттрибуты;
  3. Добавлен атрибут aria-live="assertive", который позволяет озвучивать текст в кнопке, если он поменялся по ходу работы приложения;
  4. Удалил старые скриншоты для <Link> и заменил их на более продвинутые скриншоты, которые используются в <ClickableLink>;
  5. Проп theme больше не падает на корневой тег (IF-1660);
  6. Компонент можно программно зафокусить (IF-638) — исправлено использованием :focus-visible;
  7. Подчёркивание у <Clickable as="a" view="button" отображается корректно (IF-521), но возможно этого бага в новой теме и не было, вероятно стоит просто закрыть IF-521 задачу;
  8. Добавлено состояние error для ссылки (IF-1651).

Итог:
Перенёс всю логику из <Button> и <Link> в <Clickable>, объединил и дополнил тесты для компонентов (как скриншотные, так и интеграционные), написал документацию к новому компоненту.

Ссылки

IF-969, #2598, IF-1450, IF-1661, IF-1660, IF-638, IF-521, IF-1651

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

  1. Добавлены тесты на все изменения
    ✅ unit-тесты для логики
    ✅ скриншоты для верстки и кросс-браузерности
    ⬜ нерелевантно

  2. Добавлена (обновлена) документация
    ✅ styleguidist для пропов и примеров использования компонентов
    ✅ jsdoc для утилит и хелперов
    ✅ комментарии для неочевидных мест в коде
    ⬜ прочие инструкции (README.md, contributing.md и др.)
    ⬜ нерелевантно

  3. Изменения корректно типизированы
    ✅ без использования any (см. PR 2856)
    ⬜ нерелевантно

  4. Прочее
    ✅ все тесты и линтеры на CI проходят
    ✅ в коде нет лишних изменений
    ✅ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)

@JackUait JackUait marked this pull request as draft September 14, 2023 08:50
@JackUait JackUait marked this pull request as ready for review March 4, 2024 00:14
@mshatikhin
Copy link
Member

  1. Концептуально хочется чтобы по умолчанию "Clickable" был просто button без каких либо стилей, чтобы в продуктах можно было выпилить "велосипеды Clickable"

@JackUait
Copy link
Contributor Author

Концептуально хочется чтобы по умолчанию "Clickable" был просто button без каких либо стилей, чтобы в продуктах можно было выпилить "велосипеды Clickable"

Добавил view="custom", который позволяет сбросить стили. По умолчанию оставил view="button", думаю что лучше изменять это значение на основе фидбека от пользователей. Пока что кажется, что плавный переезд со старых компонентов круче, чем view="custom" по умолчанию

/**
* Состояние валидации при ошибке.
*/
isError?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

названия isError, isWarning сломают работу с библиотекой валидаций же? напиши пару тестов на интеграцию кликабл с либой валидаций плз

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я никогда на практике не работал с библиотекой валидаций, но насколько знаю она в основном используется для валидации инпутов. Кнопки используются только для отправки формы (могу ошибаться тут). Не совсем понимаю, что тут могло сломаться. Можешь раскрыть эту тему?

/**
* Позволяет отключить контрол.
*/
isDisabled?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

старые названия без "isXX" на мой взгляд лаконичнее были, "is" писали в стародавние времена когда не было языка с типизацией, чтобы было чуть понятнее что лежит в переменной, с тайпскриптом это рудимент, лучше писать коротко и ясно, + по кодовой базе названия без is (например input), + это ломает работу с либой валидаций

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вернул названия без is

*/
size?: SizeProp;
/**
* Позвоялет перевести контрол в состояние загрузки.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

много опечаток, проверь плиз орфографию по всем файлам, например через nhunspell

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поправил орфографию

className,
)}
type={Root === 'button' ? type : undefined}
rel={getRel(rel, href)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rel в случае button бесполезен

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Оставил только для ссылки (<a />)

onBlur={handleBlur}
onClick={handleClick}
style={{ textAlign: align, ...corners, ...style }}
data-tid={ClickableDataTids.root}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

я предпочитаю сортировать пропсы, чтобы код читать и изменять было удобнее

например data-tid всегда ставлю первым атрибутом, далее системные пропсы (key, ref) далее остальные примитивные пропсы, за ними колбэки

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Думаю, что для этого лучше затащить ESLint-правило, а не править ручками
Кажется, что это лучше тащить в другом пулл-реквесте и сразу фиксить везде

/**
* Это гибридный контрол, который позволяет создавать ссылки и кнопки с любым семантическим тегом.
*/
export const Clickable: PolymorphicForwardRefExoticComponent<ClickableOwnProps, typeof CLICKABLE_DEFAULT_ELEMENT> =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

очень смущает что в исходной задаче шла речь об изменении API в Button и Link, а получился новый компонент, не ясно будущее старых компонентов, давай это обсудим на команду

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мы обсуждали это, кто-то был согласен с новым контролом, кто-то — нет. Сошлись на том, что дадим пользователям возможность протестировать новое API, если им понравится — оставим новый контрол

В текущей реализации <Button /> и <Link /> есть много откровенных костылей, которые существуют только потому что, что эти контролы взаимозаменяемы. После совмещения их в <Clickable /> эти костыли получилось выпилить. Также в рамках <Clickable /> контролы шарят довольно много логики, а также типы, если разделять их, то логику придётся копипастить в оба контрола

По поводу дальнейшей судьбы <Button />/<Link /> решили, что оставим их, но правки в них больше тащить не будем

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Button] [Link] Доработка API контролов Button и Link IF-376
3 participants