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

refactor(Button): new arrow buttons layout #2263

Merged
merged 19 commits into from
Jan 28, 2021
Merged

Conversation

StathamJason
Copy link
Contributor

С какими проблемами столкнулся:

  • Не получится просто так сделать угол более скругленным, так как теперь селектора два (:before и :after) и, соответственно, два элемента. При попытке изменения border-radius по углам происходит следующее:
    image

  • Пропала тень на верхней грани и верхнем ребре самой стрелки, так как псевдокомпоненты рисуются поверх блока с кнопкой. Если расположить под блоком, тогда надо добавлять четыре тени в box-shadow родителя, чтобы правая/левая грань была без границы и стили теней для верхнего ребра псевдоэлемента стрелки. CSS код распухнет.
    image

Новая верстка решает некоторые артефакты предыдущей, но в целом все равно получается такое себе решение

@StathamJason StathamJason changed the title Golubev/new arrow buttons refactor(Button): new arrow buttons layout Jan 11, 2021
@StathamJason
Copy link
Contributor Author

StathamJason commented Jan 11, 2021

Кажется, что визуальных артефактов стало меньше и убрались лишние большие миксины. Но угол энивэй не получится сделать круглее, остается острым.

image

@zhzz
Copy link
Member

zhzz commented Jan 12, 2021

Думаю, в целом решение получается норм и изменения того стоят. Мы избавляемся от переменных стрелок для их фона и позиционирования. Решаем проблемы с торчащими то и дело уголками. Стрелки стали проще и ровнее. 👍

По артефактам:

  1. border-radius К слову, прошлое решение его изменение вообще не поддерживало. А мы можем общий радус стрелок тоже унаследовать. Тогда до каких-то пределов его можно будет даже адекватно кастомизировать. А конкретно в углу стрелки думаю можно его захардкодить в 0 или 1px. Тогда получится что-то такое:
9px 2px
image image
  1. box-shadow Жаль, конечно, что его унаследовать никак не получится. Но это не большая беда. Артефакты, которые ты перечислил, думаю, не критичны. Но есть еще другой, по хуже. Дело в том, что стрелка перекрывает тень, которая применяется при нажатии на кнопку в дефолтной теме.
Было Стало
image image

Это стоит поправить. Думаю, как вариант обойтись копированием (с корректировкой) ее в тень верхней половинки стрелки. Будет немного заметна нестыковка при масштабировании, но вероятно с этим можно жить.

Сейчас Будет с тенью
image image
::before {
    ...
    box-shadow: 1px 0 0 0 rgba(0,0,0,0.25), inset -1px 1px 2px 0 rgba(0,0,0,0.1) !important;
}

Из других моментов, которые можно поправить до аппрува скриншотов и ревью:

  1. Изменились переменные btnDefaultHoverBorderColor и btnCheckedDisabledShadow в плоской теме. Вроде незаконно.
  2. Добавление состояния success в таблицу кнопок хорошая правка. Но сейчас она вызовет лишние изменения скриншотов, из-за которых мы можем упустить что-то важное в изменении стрелок. Лучше это сделать в конце или отдельно. Идеально, если на скриншотах поменяются только стрелки. Тогда мы будем уверены, что ничего другого не сломалось.

Еще остались переменные фона стрелок в темах, которые можно удалить.

@dzekh
Copy link

dzekh commented Jan 12, 2021

А что если внутреннюю тень нажатой кнопки сделать не честной тенью а градиентом? У нас же как-то реализован градиент на дефолтном состоянии? Тут он тоже будет выглядеть почище, если его получится подогнать.

@StathamJason
Copy link
Contributor Author

С градиентом будет видно грани псевдоэлементов. Примерно так
image

@zhzz
Copy link
Member

zhzz commented Jan 12, 2021

А что если внутреннюю тень нажатой кнопки сделать не честной тенью а градиентом? У нас же как-то реализован градиент на дефолтном состоянии? Тут он тоже будет выглядеть почище, если его получится подогнать.

Интересный вариант) Но получается не очень.

image

image

(справа все через градиенты, на второй диапазон градиента пошире)

Тень выглядит более размазанной и плавной. И плюс отбрасывается сразу на четыре стороны. А градиент нужен будет для каждой стороны отдельный.

Это похоже на нашу обводку через тени. Тоже какой-то не совсем правильный путь.

@zhzz
Copy link
Member

zhzz commented Jan 13, 2021

@dzekh а тебе не нравится мой вариант с тенью?

Вот этот

image

Он хорош еще тем, что затрагивает только сами стрелки. Сама кнопка никак не страдает.

@StathamJason
Copy link
Contributor Author

StathamJason commented Jan 14, 2021

  • Были убраны лишние миксины
  • Верстка стала попроще
  • Стало меньше визуальных артефактов
  • Бэкграунд теперь наследуется
  • Стало намного меньше переменных стрелок (и теперь они общие для левой и правой)
  • Убрана лишняя тень кнопок во флэт теме checked состояния
  • Тень верхней грани внутри стрелки задается с помощью linear-gradient через css-свойсто background-image (переменные arrowActiveShadowGradient и arrowCheckedShadowGradient)

@StathamJason StathamJason requested review from lossir and zhzz January 15, 2021 06:17
@StathamJason StathamJason marked this pull request as ready for review January 15, 2021 08:53
packages/react-ui/components/Button/Button.styles.ts Outdated Show resolved Hide resolved
packages/react-ui/components/Button/Button.mixins.ts Outdated Show resolved Hide resolved
packages/react-ui/components/Button/Button.styles.ts Outdated Show resolved Hide resolved
packages/react-ui/components/Button/Button.styles.ts Outdated Show resolved Hide resolved
packages/react-ui/internal/ThemePlayground/darkTheme.ts Outdated Show resolved Hide resolved
packages/react-ui/internal/ThemePlayground/darkTheme.ts Outdated Show resolved Hide resolved
packages/react-ui/internal/themes/FlatTheme.ts Outdated Show resolved Hide resolved
packages/react-ui/components/Button/Button.styles.ts Outdated Show resolved Hide resolved
Copy link
Member

@lossir lossir left a comment

Choose a reason for hiding this comment

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

Из-за изменений, о которых Егор говорит здесь #2263 (comment), также пришлось обновить скриншоты теста Button/text styles reset. Их тоже надо вернуть.


У стрелки совсем нет внутренней тени в нажатом состоянии. Мне кажется, её стоит добавить. Я попробовал сделать вариант, правда, только через браузер.

где чего
было image
стало image
как вариант image

packages/react-ui/internal/themes/DefaultTheme.ts Outdated Show resolved Hide resolved
@dzekh
Copy link

dzekh commented Jan 19, 2021

У стрелки совсем нет внутренней тени в нажатом состоянии. Мне кажется, её стоит добавить. Я попробовал сделать вариант, правда, только через браузер.

Да, мне нравится такой вариант, который «как вариант».
Если он не усложнит особо, и не добавит новых переменных.

lossir
lossir previously approved these changes Jan 20, 2021
Copy link
Member

@lossir lossir left a comment

Choose a reason for hiding this comment

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

По моим замечаниям всё ок. После переапрува скриншотов ещё гляну.

Copy link
Member

@zhzz zhzz left a comment

Choose a reason for hiding this comment

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

Переаппрувлено и добавлено несколько лишних скриншотов (DateInput, Input, Link, Radio, Textarea, Tooltip). В идеале, должны поменяться только скриншоты с кнопками-стрелками. Новых быть не должно, т.к. тестов не добавляли.

Все это видно на вкладке "Files changed" в ПРе.

Copy link
Member

@zhzz zhzz left a comment

Choose a reason for hiding this comment

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

Теперь четко 👍

@lossir lossir merged commit bf1e921 into next Jan 28, 2021
@lossir lossir deleted the golubev/new-arrow-buttons branch January 28, 2021 07:34
@zhzz zhzz mentioned this pull request Feb 9, 2021
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.

4 participants