BOX-169 BOX-154 adds theme const for autocomplete #47

OlegBrony · 2021-08-04T19:46:59Z

for colors

ISSUES CLOSED: #169

for colors ISSUES CLOSED: #169

src/lib/theme.ts

src/entities/card/organisms/card-preview.tsx

azinit · 2021-08-06T20:01:08Z

@OlegBrony поправишь?

OlegBrony · 2021-08-06T20:32:28Z

кого?

azinit · 2021-08-07T12:33:30Z

@dmi-ch @asvtsv @antonmazhuto @Kater-auf-Dach гляньте пож)

* refactor(app): adds css custom props for other repeatable colors #154 ISSUES CLOSED: #154 * chore(app): fix extra item in commitizen config ISSUES CLOSED: #154

# Conflicts: # src/entities/user/ui/user-card.tsx # src/entities/user/ui/user-preview.tsx # src/features/search-bar/organisms/search-bar.tsx # src/pages/card/view/index.tsx # src/pages/comments/comments.tsx # src/pages/home/index.tsx # src/pages/search/page.tsx # src/pages/user/index.tsx # src/pages/user/skeleton.tsx # src/ui/atoms/button.tsx

BREAKING CHANGE: css theme (removed some variables) ISSUES CLOSED: #169

src/lib/theme.ts

azinit

Ох уж точно, "лучше бы влили как есть"

Теперь еще больше вопросов 😄

src/lib/theme.ts

azinit · 2021-08-15T19:19:43Z

src/lib/theme.ts

+  successDisabled: '--success800',
+  success750: '--success750',
+  success700: '--success700',
+  success650: '--success650',
+  success600: '--success600',
+  success550: '--success550',
+  success500: '--success500',
+  success: '--success500',
+  success450: '--success450',
+  success400: '--success400',
+  successHover: '--success400',
+  success350: '--success350',
+  success300: '--success300',
+  successPressed: '--success300',


nitpick(non-blocking): Мб не "Pressed", а "Active"? (здесь и в других местах)

Hover по сути тот же Pressed почти

hover - не pressed
на active тогда заменить hover, потому что active - это как бы и hover, и focus, звучит логично. но pressed всё же стоит отдельно добавить, я считаю.

active - это как бы и hover, и focus

Не всегда)

ну да, но уж точно pressed не будет одинаковым ни с hover, ни с focus.
а вот hover вполне себе может быть идентичен focus, и это не будет ломать представление ui.

Ну оке, посмотрим что остальные скажут)
Мне не особо критично

src/lib/theme.ts

src/pages/comments/comments.tsx

azinit · 2021-08-15T19:24:50Z

src/app/application.tsx

+  :root {
+    --wizard1000: #FFF;
+    --wizard950: #F6F5FF;
+    --wizard900: #E8E6FE;
+    --wizard850: #D6D2FE;
+    --wizard800: #C0B9FE;
+    --wizard750: #A59BFD;
+    --wizard700: #8A7DFC;
+    --wizard650: #6F5FFC;


issue: Слуш, я мб проморгал сначала - но щас явно заметно, что при большом количестве css-переменных, декларирование глобальной палитры превращается в скакание между двумя слоями на разных полюсах - shared и app (и это не есть хорошо)

suggestion: Мб подумаем над вариантом, чтобы занести эту логику в shared/lib/theme?

(чтобы там же был доступ к названиям переменных, и там же можно было одной функцией получить все переменные для такой декларции - чтобы ось изменений локализоана была)

Условно, можно хранить большую мапу <cssVarName, colorValue>

И отдельно хранить те маппы к названиям, которые уже есть

И отдельно можно сделать функцию, которая бы прошлась по всем переменным из палитры и превратила бы во что-то такое

(а то кажется рили не оч, чтобы эта логика размывалась в двух настолько разных местах)

ну звучит вроде неплохо, хотя не до конца понял, что именно ты хочешь.

Ну тебе надо просто переложить логику создания палитры в shared/lib/theme

Я предложил лишь одну из реализаций, но можешь действовать по своему усмотрению

src/entities/user/ui/user-card.tsx

azinit · 2021-08-15T19:34:26Z

src/features/card/draft/ui/atoms/reset-changes.tsx

+const CANCEL_WARN =
+  'Are you sure you want to undo the changes? The action is not reversible!';


thoughts: Это не просто так дизейблилось 😄

а зочем?

Потому что для длинных строк не имеет смысла такой перенос

😐
сказал сначала так, будто от этого поведение меняется. как по мне так лучше - если оно вмещается в ширину экрана (которая диктуется линтером), то всё хорошо. мне, например, очень не нравится x-scroll.

У тебя такой перенос спасает ситуацию на ~ 5-10 символов (в зависимости от длины переменной)

А дальше? Скролла не избежать

Если только ты не додумаешься бить строку по частям - но это еще херовей

какое-то обречённое мнение. скрола можно и нужно избежать. там, где не получается - это скорее всего означает, что скролить не нужно (например, длинные сообщения не несущие логики)

началось оно, кста, с того, что дизейбл линта не дизейблит притиер.

мнение. скрола можно и нужно избежать.

что скролить не нужно (например, длинные сообщения не несущие логики)

Ты сам себе противоречишь)

что дизейбл линта не дизейблит притиер.

Именно, но это пофиксится по тикету другому

И тут как раз пример "когда не надо скролить"

src/features/search-bar/molecules/search.tsx

src/pages/user/skeleton.tsx

src/ui/molecules/title.tsx

BREAKING CHANGE: css theme (removed some variables) ISSUES CLOSED: #169 ISSUES CLOSED: #169

BREAKING CHANGE: css theme (removed some variables) ISSUES CLOSED: #169 ISSUES CLOSED: #169 ISSUES CLOSED: #169

azinit · 2021-08-18T11:33:35Z

src/lib/theme.ts

+// in case of modifying the palette you can only modify this object
+// without modifying the rest
+const customPropsObject = {
+  wizard: {
+    wizard1000: '#fff',
+    wizard950: '#f6f5ff',
+    wizard900: '#e8e6fe',
+    wizard850: '#d6d2fe',
+    wizard800: '#c0b9fe',
+    wizard750: '#a59bfd',
+    wizard700: '#8a7dfc',
+    wizard650: '#6f5ffc',
+    wizard600: '#5441fb',
+    wizard550: '#3923fb',
+    wizard500: '#1e05fa',
+    wizard450: '#1a04dc',


thoughts: Не оч зашла реализация, кмк громоздкая вышла

Но возможно вкусовщина моя личная, поэтому в тикет вынес - главное, что работает)

praise: Все круто сделал!

https://linear.app/atomix/issue/BOX-211/uprostit-po-vozmozhnosti-generaciyu-palitry

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

fix(app): adds theme const

df67785

for colors ISSUES CLOSED: #169

OlegBrony requested a review from sergeysova August 4, 2021 19:46

OlegBrony self-assigned this Aug 4, 2021

OlegBrony requested a review from azinit August 4, 2021 19:47

azinit previously approved these changes Aug 4, 2021

View reviewed changes

src/lib/theme.ts Outdated Show resolved Hide resolved

src/entities/card/organisms/card-preview.tsx Outdated Show resolved Hide resolved

azinit mentioned this pull request Aug 6, 2021

BOX-154: Replaces some repeatable css colors with custom props #48

Merged

BOX-154: Replaces some repeatable css colors with custom props (#48)

45d1fd0

* refactor(app): adds css custom props for other repeatable colors #154 ISSUES CLOSED: #154 * chore(app): fix extra item in commitizen config ISSUES CLOSED: #154

azinit dismissed their stale review via 45d1fd0 August 13, 2021 20:24

azinit mentioned this pull request Aug 15, 2021

BOX-179: Refine UIKit implementation and API usage #58

Merged

OlegBrony added 4 commits August 15, 2021 20:12

Merge branch 'master' into BOX-169

4c5aa59

Merge branch 'master' into BOX-169

509282e

Merge branch 'master' into BOX-169

667ae3d

fix(app): added color schemas from figma

4e57a49

BREAKING CHANGE: css theme (removed some variables) ISSUES CLOSED: #169

OlegBrony commented Aug 15, 2021

View reviewed changes

src/lib/theme.ts Outdated Show resolved Hide resolved

azinit previously approved these changes Aug 15, 2021

View reviewed changes

azinit requested review from asvtsv, antonmazhuto and Kater-auf-Dach August 15, 2021 19:38

azinit changed the title ~~fix(app): adds theme const for autocomplete~~ BOX-169 BOX-154 adds theme const for autocomplete Aug 15, 2021

antonmazhuto previously approved these changes Aug 16, 2021

View reviewed changes

fix(app): fix colors after review

b357208

BREAKING CHANGE: css theme (removed some variables) ISSUES CLOSED: #169 ISSUES CLOSED: #169

OlegBrony dismissed stale reviews from antonmazhuto and azinit via b357208 August 16, 2021 15:31

fix(app): moves theme styles to lib

2321b71

BREAKING CHANGE: css theme (removed some variables) ISSUES CLOSED: #169 ISSUES CLOSED: #169 ISSUES CLOSED: #169

azinit approved these changes Aug 18, 2021

View reviewed changes

Kater-auf-Dach approved these changes Aug 18, 2021

View reviewed changes

azinit merged commit 00366a3 into master Aug 18, 2021

azinit deleted the BOX-169 branch August 18, 2021 11:37

sergeysova added the Improvement label Oct 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BOX-169 BOX-154 adds theme const for autocomplete #47

BOX-169 BOX-154 adds theme const for autocomplete #47

OlegBrony commented Aug 4, 2021

azinit commented Aug 6, 2021

OlegBrony commented Aug 6, 2021

azinit commented Aug 7, 2021

azinit left a comment

azinit Aug 15, 2021

OlegBrony Aug 15, 2021

azinit Aug 15, 2021

OlegBrony Aug 15, 2021 •

edited

Loading

azinit Aug 15, 2021

azinit Aug 15, 2021

azinit Aug 15, 2021

OlegBrony Aug 15, 2021

azinit Aug 15, 2021

azinit Aug 15, 2021

OlegBrony Aug 15, 2021

azinit Aug 15, 2021

OlegBrony Aug 15, 2021

azinit Aug 15, 2021

OlegBrony Aug 16, 2021

OlegBrony Aug 16, 2021

azinit Aug 17, 2021

azinit Aug 17, 2021

azinit Aug 18, 2021

OlegBrony Aug 18, 2021 •

edited

Loading

		const CANCEL_WARN =
		'Are you sure you want to undo the changes? The action is not reversible!';

BOX-169 BOX-154 adds theme const for autocomplete #47

BOX-169 BOX-154 adds theme const for autocomplete #47

Conversation

OlegBrony commented Aug 4, 2021

azinit commented Aug 6, 2021

OlegBrony commented Aug 6, 2021

azinit commented Aug 7, 2021

azinit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegBrony Aug 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegBrony Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

OlegBrony Aug 15, 2021 •

edited

Loading

OlegBrony Aug 18, 2021 •

edited

Loading