-
Notifications
You must be signed in to change notification settings - Fork 20
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
plasma-new-hope: Refactoring tabs tokens, variations, config #873
Conversation
Theme Builder app deployed! http://plasma.sberdevices.ru/pr/plasma-theme-builder-pr-873/ |
Documentation preview deployed! website: http://plasma.sberdevices.ru/pr/pr-873/ |
--plasma-tabs-color: var(--text-primary); | ||
--plasma-tabs-color-hover: var(--text-primary); | ||
--plasma-tabs-font-color: var(--text-primary); | ||
--plasma-tabs-font-color-hover: var(--text-primary); |
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.
Поправил
@@ -12,22 +12,22 @@ export const config = { | |||
--plasma-tabs-underline-color: var(--surface-solid-tertiary); | |||
--plasma-tabs-underline-height: 2px; | |||
--plasma-tabs-font-weight: 500; | |||
--plasma-tabs-color: var(--text-accent); | |||
--plasma-tabs-font-color: var(--text-accent); |
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.
не критично
кажется, что не обязательно добавлять font
, т.к. именно для фона какого-то элемента (в данном случае таба) используется свойство background, и выглядело бы оно тогда --plasma-tabs-background
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.
Поправил
fontWeight: '--plasma-tabs-font-weight', | ||
fontColor: '--plasma-tabs-font-color', | ||
fontColorHover: '--plasma-tabs-font-color-hover', | ||
}; |
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.
а вообще подумал, что лучше эти токены перенести в общий файлик - Tabs.tokens
и забирать из одного места во всех компонентах
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.
Поправил
b6703a9
to
7d4ccdb
Compare
Theme Builder app deployed! http://plasma.sberdevices.ru/pr/plasma-theme-builder-pr-873/ |
Documentation preview deployed! website: http://plasma.sberdevices.ru/pr/pr-873/ |
--plasma-tabs-cursor: pointer; | ||
--plasma-tabs-color-hover: var(--text-primary); | ||
${tokens.underlineColor}: var(--surface-solid-tertiary); | ||
${tokens.underlineHeight}: 2px; |
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.
переведи в rem пожалуста
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.
Поправил
import { css } from '@linaria/core'; | ||
|
||
import { tokens } from '../../../../components/Tabs/TabItem.tokens'; |
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.
здесь лучше забрать токены из индекса, по аналогии с Dropdown:
import { dropdownTokens } from '../../../../../components/Dropdown';
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.
Поправил
@@ -1 +1,2 @@ | |||
export { tabsRoot, tabsConfig } from './Tabs'; | |||
export { tabItemRoot, tabItemConfig } from './TabItem'; |
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.
тут ещё нужно добавить ре-экспорт токенов
export { classes as dropdownClasses, tokens as dropdownTokens } from './Dropdown.tokens';
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.
Поправил
:hover { | ||
cursor: var(${tokens.cursor}); | ||
} | ||
`; |
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.
кажется, что это должно быть не в disabled
? 🤔
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.
Удалил по аналогии с active
@@ -0,0 +1,4 @@ | |||
export const tokens = { | |||
disabledOpacity: '--plasma-tabs-disabled-opacity', | |||
containerWidth: '--plasma-tabs-container-width', |
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.
вот сюда добавить токены из TabsItem, имел ввиду
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.
Перенес
7d4ccdb
to
7576816
Compare
Theme Builder app deployed! http://plasma.sberdevices.ru/pr/plasma-theme-builder-pr-873/ |
Documentation preview deployed! website: http://plasma.sberdevices.ru/pr/pr-873/ |
🎱 |
🚀 This PR is included in version: @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected] 🚀 |
Tabs
What/why changed
🐤 Download canary assets:
📦 Published PR as canary version:
Canary Versions
✨ Test out this PR locally via: