Skip to content

Компонент Button плохо работает с пропсом Component #824

Open
ekabolotina opened this issue Aug 31, 2021 · 9 comments
Assignees
Labels
Enhancement New feature or request Help Wanted Extra attention is needed

Comments

@ekabolotina
Copy link
Contributor

ekabolotina commented Aug 31, 2021

Опишите проблему

Есть необходимость сделать кнопку ссылкой на внутреннюю страницу. Переход по ссылке не должен перезагружать страницу. Т.к. в проекте используется react-router, то для переходов используется компонент Link из react-router-dom. Если в качестве пропса Component передать компонент Link, то возникают 2 проблемы:

  1. Typescript не дает возможность использовать пропсы, необходимые для Link, на компоненте Button
  2. Компонент Button добавляет ненужный (невалидный) пропс type="button", хотя никакая кнопка в моем случае не рендерится.

Ожидаемое поведение

Если задан пропс Component, то компонент Button должен уметь принимать все необходимые пропсы для компонента Component и прокидывать их в него. Также если задан пропс Component, то компонент Button не должен добавлять ненужные пропсы типа type="button".

Тестовый стенд

https://codesandbox.io/s/lucid-benz-oj9pu?file=/src/App.tsx

@reme3d2y
Copy link
Contributor

reme3d2y commented Aug 31, 2021

@ekabolotina Поменяй to на href. Мы эту задачу катили в срочном режиме, поэтому забыли добавить доку.

Component нужен в 99% для интеграции с роутером, поэтому нам нужно было дать возможность отрендерить Link внутри нашей кнопки + накинуть to. Обсудили с ребятами из клика это, сошлись на том, что можно не добавлять to, а оставить href.

@ekabolotina
Copy link
Contributor Author

Костыль-то я сделал, здесь вопрос в том, чтобы доработать этот компонент так, чтобы он работал правильно.

@reme3d2y
Copy link
Contributor

reme3d2y commented Aug 31, 2021

А что сейчас работает неправильно? Какой твой кейс текущее решение не покрывает и какие пропсы ты хочешь прокинуть в кастомный компонент?

Я не вижу причин делать из кнопки дженерик и заморачиваться с типами. Сейчас, чтобы интегрировать кнопку с роутером, достаточно добавить Component={Link} и больше ничего делать не нужно. Я и не могу представить другие кейсы, когда нужен кастомный компонент и какие-то дополнительные пропсы.

Но мы всегда открыты для контрибьюта, поэтому если есть желание что-то улучшить — welcome ✋

@ekabolotina
Copy link
Contributor Author

Все проблемы я написал в начале темы. Текущая реализация это какой-то костыль. Плюс, чтобы она работала, нужно локальный костыль дописать, что я и сделал. Если это ок, то вопросов нет. Но учитывая, что это общедоступная библиотека, заявленная как (см. скриншот), то это не ок.

image

@reme3d2y
Copy link
Contributor

Ты берешь Button, ставишь ему href и в качестве Component передаешь Link, все работает как нужно.
Я не понимаю, какой локальный костыль тебе нужен для этого. Что ты называешь костылем?

Это общебанковская либа и ты можешь принести ПР, если что-то по-твоему мнению сделано не так.
Не понимаю твоей претензии вообще.

@ekabolotina
Copy link
Contributor Author

ekabolotina commented Aug 31, 2021

С какой стати href превращается в to внутри компонента? А если я буду использовать не Link, а какой-то другой кастомный компонент? Понятно, что это не самый частый случай, но это же core компоненты, они должны уметь работать в общем, а не в частном, случае.

Насчет предложить ПР — я не против помочь. С другой стороны я не понимаю твоего сопротивления. Понятно, что вы выкатили быструю фичу, но это не повод отказываться сделать доработку и положить, например, эту задачу в бэклог.

Все то же самое нужно будет и для компонента IconButton.

@reme3d2y
Copy link
Contributor

Обсудили в личке, сделаем ПР

@reme3d2y reme3d2y added Enhancement New feature or request Proposal labels Aug 31, 2021
@reme3d2y
Copy link
Contributor

@ekabolotina Вань, нужна помощь. Я уже который раз пробую взять эту таску, но у меня не получается подружить это все с forwardRef

@ekabolotina
Copy link
Contributor Author

@ekabolotina Вань, нужна помощь. Я уже который раз пробую взять эту таску, но у меня не получается подружить это все с forwardRef

понял, постараюсь помочь)

@reme3d2y reme3d2y added Help Wanted Extra attention is needed and removed Proposal labels Jan 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement New feature or request Help Wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants