[DS-22] feature: implement line-box, refactor checkbox and radio-button #185
Conversation
ainursharaev
requested review from
rchubarkin,
tatinacher,
risen228 and
Irinaristova
src/woly/elements/box/index.tsx
Outdated
| padding-top: var(--local-vertical); | ||
| padding-bottom: var(--local-vertical); | ||
| } | ||
| padding-top: var(--local-vertical); |
There was a problem hiding this comment.
nitpick: тут можно в одну строку + проверь что с горизонтальными паддингами происходит, чтобы браузер свои не добавил
| &:focus [data-element='checkmark'] > svg, | ||
| &:active [data-element='checkmark'] > svg { | ||
| &:focus [data-element^='checkbox-'] > svg, | ||
| &:active [data-element^='checkbox-'] > svg { | ||
| box-shadow: 0 0 0 var(--woly-border-width) var(--woly-focus); |
There was a problem hiding this comment.
suggestion: тут название переменной должно быть var(--woly-focus-color)
| input { | ||
| display: none; | ||
| } | ||
| margin-right: var(--local-gap); |
There was a problem hiding this comment.
suggestion: зачем выносить это сюда? лучше управление gaps зашить внутрь формулы linedox
|
|
||
| outline: none; | ||
| } | ||
|
|
||
| [data-element='container'] { | ||
| display: flex; |
There was a problem hiding this comment.
suggestion: мб лучше это тоже зашить в формулу? элементы всегда будут расположены горизонтально, поэтому отдавать компоненту тут управление расположением наверно не имеет смысла
| } | ||
| &:focus, | ||
| &:active { | ||
| --local-icon-fill: var(--woly-focus); |
There was a problem hiding this comment.
suggestion: название переменной цвета надо поправить
|
|
||
| outline: none; | ||
|
|
||
| user-select: none; | ||
|
|
||
| &:focus [data-element='checkmark'] > svg, | ||
| &:active [data-element='checkmark'] > svg { | ||
| &:focus [data-element^='checkbox-'] > svg, |
There was a problem hiding this comment.
nitpick: мб только на фокус оставим, как в radio-button?
|
|
||
| export const Box = styled.div` | ||
| ${box} | ||
| ` as StyledComponent<'div', Record<string, unknown>>; | ||
|
|
||
| export const LineBox = styled.div` |
There was a problem hiding this comment.
question: Можешь плиз подробнее рассказать о юзкейсах для этого компонента, какие проблемы он решает и т.д.
There was a problem hiding this comment.
Мы экспортируем box-ы через такие обертки, чтобы пользователь мог использовать их в своих компонентах. К примеру, можно создать свои line-компоненты:
<LineBox>
<Avatar />
<Name />
<LineBox>
ainursharaev
force-pushed
the
feat/DS-22-implement-line-box
branch
from
6790922 to
916b1d6
Compare
| @@ -0,0 +1,15 @@ | |||
| import { css } from 'styled-components'; | |||
|
|
|||
| export const visuallyHidden = css` | |||
There was a problem hiding this comment.
а что это за стили такие? почему через display none не скрыть?
There was a problem hiding this comment.
согласна с Ромой, плюс отрицательный маргин использовать, как мне кажется, не очень хорошая затея
There was a problem hiding this comment.
Это для доступности. Подробнее можно прочитать тут
There was a problem hiding this comment.
прикрепи плиз этот линк в комментарии к коду а то нифига не понятно
There was a problem hiding this comment.
question: мб добавить эти стили в WolyGlobalStyles и юзать через атрибут data-visually-hidden?
| @@ -0,0 +1,15 @@ | |||
| import { css } from 'styled-components'; | |||
|
|
|||
| export const visuallyHidden = css` | |||
There was a problem hiding this comment.
согласна с Ромой, плюс отрицательный маргин использовать, как мне кажется, не очень хорошая затея
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| input:checked ~ [data-element='checkbox-checked'] { | ||
| svg > rect { | ||
| fill: var(--local-icon-fill); |
There was a problem hiding this comment.
suggestion: цвет на 130 строке должен быть var(--woly-focus-color)
| checked={checked} | ||
| onChange={onChange} | ||
| onKeyDown={onKeyDown} | ||
| disabled={disabled} |
There was a problem hiding this comment.
suggestion: мне кажется не очень ок прокидывать 2 disabled в компонент, но можем обсудить это командой, мне кажется, есть более удобное решение
There was a problem hiding this comment.
Закинул вопрос на обсуждение
| )} | ||
| </State> | ||
| </Playground> | ||
| ```tsx playground |
ainursharaev
dismissed stale reviews from risen228, sergeysova, and Irinaristova
via
9b2b784
ainursharaev
force-pushed
the
feat/DS-22-implement-line-box
branch
from
cd100e9 to
9b2b784
Compare
Link to issue
What have I done:
What have to be done: