-
Notifications
You must be signed in to change notification settings - Fork 7
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
BOX-169 BOX-154 adds theme const for autocomplete #47
Changes from 7 commits
df67785
45d1fd0
da2d27b
4c5aa59
509282e
667ae3d
4e57a49
b357208
2321b71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,8 @@ import { useEvent } from 'effector-react/ssr'; | |
|
||
import * as model from '../../model'; | ||
|
||
// eslint-disable-next-line prettier/prettier | ||
const CANCEL_WARN = 'Are you sure you want to undo the changes? The action is not reversible!'; | ||
const CANCEL_WARN = | ||
'Are you sure you want to undo the changes? The action is not reversible!'; | ||
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. а зочем? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Потому что для длинных строк не имеет смысла такой перенос There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😐 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. У тебя такой перенос спасает ситуацию на ~ 5-10 символов (в зависимости от длины переменной) А дальше? Скролла не избежать
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. какое-то обречённое мнение. скрола можно и нужно избежать. там, где не получается - это скорее всего означает, что скролить не нужно (например, длинные сообщения не несущие логики) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. началось оно, кста, с того, что дизейбл линта не дизейблит притиер. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ты сам себе противоречишь) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Именно, но это пофиксится по тикету другому И тут как раз пример "когда не надо скролить" |
||
|
||
interface Props { | ||
_name: string; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue:
Слуш, я мб проморгал сначала - но щас явно заметно, что при большом количестве css-переменных, декларирование глобальной палитры превращается в скакание между двумя слоями на разных полюсах - shared и app (и это не есть хорошо)suggestion:
Мб подумаем над вариантом, чтобы занести эту логику в shared/lib/theme?(чтобы там же был доступ к названиям переменных, и там же можно было одной функцией получить все переменные для такой декларции - чтобы ось изменений локализоана была)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Условно, можно хранить большую мапу
<cssVarName, colorValue>
И отдельно хранить те маппы к названиям, которые уже есть
И отдельно можно сделать функцию, которая бы прошлась по всем переменным из палитры и превратила бы во что-то такое
(а то кажется рили не оч, чтобы эта логика размывалась в двух настолько разных местах)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ну звучит вроде неплохо, хотя не до конца понял, что именно ты хочешь.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ну тебе надо просто переложить логику создания палитры в
shared/lib/theme
Я предложил лишь одну из реализаций, но можешь действовать по своему усмотрению