-
Notifications
You must be signed in to change notification settings - Fork 46
WIP themer #977
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
base: master
Are you sure you want to change the base?
WIP themer #977
Conversation
src/constants.ts
Outdated
| export const MAIN_TIMER_ID = 'Build time'; | ||
| export const YFM_CONFIG_FILENAME = '.yfm'; | ||
| export const THEME_CONFIG_FILENAME = 'theme.yaml'; | ||
| export const THEME_CSS_PATH = '_assets/style/theme.css'; // Какие есть варианты где должен быть этот файл? |
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.
давай добавим _ чтобы показать, что это не пользовательский файл _theme.css
src/steps/processThemer.ts
Outdated
| } | ||
| } | ||
|
|
||
| function createTheme(configData: ThemeConfig): 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.
строки 'light' и 'dark' будет лучше перенести в константы, они в этом файле часто используются
src/steps/themer/types.ts
Outdated
| 'note-tip-background', | ||
| 'note-warning-background', | ||
| 'note-important-background', | ||
| 'hljs-addition', // нужно ли давать менять hljs переменные? |
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.
'hljs-addition', // нужно ли давать менять hljs переменные?
нет
src/steps/themer/types.ts
Outdated
| export interface ColorsOptions extends GravityColorsOptions, YFMColorOptions {} | ||
|
|
||
| export const YFM_COLOR_KEYS = [ | ||
| 'note-info-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.
мало yfm цветов, пробежался по разделу "синтаксис" по доке с со сбилженной темой, нельзя поменять
- цвет иконок у заметок
- цвет блока кода
- линии табов
- фон и обводка попапов
- фон у таблиц и тд
посмотри в нашей документации для каких элементов еще можно добавить изменение цвета.
Если у yfm, элемента нет своей css переменной, надо сначала ее добавить в трансформ. И убрать переопределение цвета этого элемента из components.
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.
Для каждого элемента в yfm у которого мы меняем цвет должна быть своя переменная. Например, фон попапов --yfm-color-term-dfn-background с оригинальным цветом
https://github.com/diplodoc-platform/transform/blob/master/src/scss/_term.scss#L35
В components мы заменяем оригинальный цвет на цвета из гравити
https://github.com/diplodoc-platform/components/blob/master/src/styles/yfm.scss#L74
тут только должен быть не селектор через dfn, а замена цвет через переменную --yfm-color-term-dfn-background
и уже в клишке когда themer меняет цвет, мы меняем только цвет у переменной --yfm-color-term-dfn-background
src/steps/themer/validator.ts
Outdated
| light: themeProperties, | ||
| dark: themeProperties, | ||
| }, | ||
| required: ['light', 'dark'], |
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.
'light', 'dark' не должны быть обязательными полями, мы должны иметь возможность указать цвета только для одно из этих значений. Вместе с этим, если цвета для темной и светлой темы одинаковые, должна быть возможность не дублировать их в 'light', 'dark' а указать только один раз в конфиге на верхнем уровне
src/commands/build/handler.ts
Outdated
|
|
||
| await processChangelogs(); | ||
|
|
||
| await processThemer(run); |
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.
наличие конфига не должно быть обязательным, сейчас получаем ошибку если theme.yaml нет в доке
d81a901 to
0e15f40
Compare
5ebbfd3 to
dadb78a
Compare
61526b7 to
238c466
Compare
|
|
||
| import {resolve} from 'node:path'; | ||
| import {THEME_CONFIG_FILENAME, THEME_CSS_PATH} from '~/constants'; | ||
| import * as yaml from 'js-yaml'; |
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 { load } from 'js-yaml';
| export function createTheme(configData: ThemeConfig): Theme { | ||
| const theme: Theme = {}; | ||
|
|
||
| // if (configData['base-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.
этот код нужен?
| .AfterRun.for('md') | ||
| .tapPromise('Themer', async (run) => { | ||
| if (isThemeExists(run.input)) { | ||
| await run.copy( |
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.
для md прогона нам тоже нужно сгенерировать css с темой
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.
Так же в ту же папку _assets/style?
|
тесты ? |
|
не вижу обновлений в трансформе - тебе не пришлось добавить туда ни одной переменной, связанной с цветом? и к этому ПРу нужно еще добавить ПР с докой для этой фичи |
3de49c4 to
238c466
Compare
| colors: { | ||
| 'base-brand': 'rgb(94 33 41)', | ||
| 'base-background': 'rgb(255,255,255)', | ||
| 'base-brand-hover': 'var(--g-color-private-brand-650-solid)', |
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.
все эти значения нужно взять из констант, которые ты уже опеределил
|
|
||
| describe('Build themer feature', () => { | ||
| describe('Apply theme', () => { | ||
| generateMapTestTemplate('md2md with theme yaml', '__tests__/mocks/md2md-with-theme-yaml', { |
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.
давай разделим юнит тесты для самой функции генерации и снепшот тесты результата билда
| theme: false, | ||
| }); | ||
|
|
||
| test('should handle arg', "--theme 'note-info-background: rgb(40, 216, 105)'", { |
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.
через --theme должна быть возможность передать только базовый цвет --theme red, а подробная настройка должна быть через конфиг
46acb0a to
b4bcc4d
Compare
52f2688 to
50a4135
Compare
2698a95 to
cfe7e59
Compare
Warning! Work in progress