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

chore: remove old themes and theme specific styles #3459

Open
wants to merge 135 commits into
base: 5.x
Choose a base branch
from

Conversation

Funkicide
Copy link
Member

Проблема

В рамках мажорного релиза принято решение удалить все старые темы.

Решение

  • Смержил апдейт цветов в 22е темы.
  • Удалил deprecated переменные.
  • Удалил старые темы, кроме default theme и dark theme.
  • Задепрекейтил default theme и dark theme. Теперь они ссылаются внутри на theme2022 и theme2022dark соответственно.
  • Удалил версту и стили, специфичные для старых тем.

Ссылки

resolve IF-1791

Чек-лист перед запросом ревью

  1. Добавлены тесты на все изменения
    ⬜ unit-тесты для логики
    ✅ скриншоты для верстки и кросс-браузерности
    ⬜ нерелевантно

  2. Добавлена (обновлена) документация
    ⬜ styleguidist для пропов и примеров использования компонентов
    ⬜ jsdoc для утилит и хелперов
    ⬜ комментарии для неочевидных мест в коде
    ✅ прочие инструкции (README.md, contributing.md и др.)
    ⬜ нерелевантно

  3. Изменения корректно типизированы
    ⬜ без использования any (см. PR 2856)
    ✅ нерелевантно

  4. Прочее
    ⬜ все тесты и линтеры на CI проходят
    ⬜ в коде нет лишних изменений
    ✅ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)

Funkicide and others added 30 commits June 17, 2024 17:05
fileUploaderDisabledIconColor: '#ADADAD',
};

export const THEME_2022 = applyMarkers(
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Не понял что за лишние изменения?

@@ -37,8 +36,6 @@ const customViewports = {
const themes = {
DEFAULT_THEME,
Copy link
Member Author

Choose a reason for hiding this comment

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

Не уверен, стоит ли удалять дефолтные темы отсюда и из функции decorators ниже.

Copy link
Member

Choose a reason for hiding this comment

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

Надо удалить. Если мы и оставим DEFAULT_THEME, то это будет простой редирект на THEME_2022.
И чисто для пользователей где-то в packages/react-ui/lib/theming/themes

@Funkicide Funkicide marked this pull request as ready for review July 5, 2024 12:38
@lossir
Copy link
Member

lossir commented Jul 18, 2024

1. Кривой Тултип в доке

Если в доке зайти на страницу http://localhost:6060/#/Customization/ThemePlayground,
а затем на любую другую страницу, то появляется такой Тултип.
image

Иногда после перехода на другую страницу надо немного поскроллить слева.

Если на него кликнуть, то валится такая ошибка:

image

2. Удалить старые иконки

Из папки packages/react-ui/internal/icons надо вытащить SpinnerIcon в отдельную папку, напр.
packages/react-ui/internal/SpinnerIcon

Все остальные иконки в папке надо удалить.

Если они где-то ещё используются, то надо заменить на эквиваленты из соседней папки icons2022.
Например, старая иконка CrossIcon ещё используется в SideMenu. Вместо неё надо применить специальный компонент CloseButtonIcon. Если не получится, то сделать как в Модалке.

Саму папку можно переименовать в icons

3. Таблица удалённых переменных

Нужна таблица со всеми удаленными переменными, и их новыми эквивалентами.
Если эквивалентов нет, то это должно быть указано. Напр, в DatePicker вместо специальных переменных для кнопки "сегодня" можно использовать переменные обычной кнопки.
Придётся конечно обернуть ДатаПикер в отдельный контекст. Мб ещё успеем добавить в 5.0 проп theme везде.

4. Кодмод для переменных тем

Для переменных, вместо которых появились однозначные эквиваленты, надо написать кодмод.
Его можно будет написать отдельный ПРом, стоит заранее посмотреть как он делается.

Пример можешь подсмотреть тут
https://github.com/skbkontur/retail-ui/blob/next/packages/react-ui-codemod/react-ui-4.0/renameThemeVars.ts

Как им пользоваться можно посмотреть тут
#1900

5. Удалить InternalMenu (?)

Не уверен, что надо делать именно в этом ПРе.

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

fileUploaderDisabledIconColor: '#ADADAD',
};

export const THEME_2022 = applyMarkers(
Copy link
Member

Choose a reason for hiding this comment

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

Не понял что за лишние изменения?

Comment on lines 7 to 8
DEFAULT_THEME,
DARK_THEME,
Copy link
Member

Choose a reason for hiding this comment

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

Эти темы надо убрать.
К теме THEME_2022 можно сделать приписку в скобочках, типа "по-умолчанию", или покороче "default".

Comment on lines 109 to 110
<Tabs.Tab id={ThemeType.Default}>Дефолтная</Tabs.Tab>
<Tabs.Tab id={ThemeType.Dark}>Темная</Tabs.Tab>
Copy link
Member

Choose a reason for hiding this comment

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

Тут тоже надо удалить

Comment on lines 43 to -45
browsers: {
chrome8px: {
browserName: 'chrome',
Copy link
Member

Choose a reason for hiding this comment

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

Ниже надо удалить конфиги для ie.
Также ещё остались скриншоты, т.к. мы лишь закоментили конфиги. Их тоже надо удалить.

@@ -37,8 +36,6 @@ const customViewports = {
const themes = {
DEFAULT_THEME,
Copy link
Member

Choose a reason for hiding this comment

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

Надо удалить. Если мы и оставим DEFAULT_THEME, то это будет простой редирект на THEME_2022.
И чисто для пользователей где-то в packages/react-ui/lib/theming/themes

Comment on lines 470 to 485
// return (
// <label data-tid={InputDataTids.root} {...labelProps}>
// <span className={styles.sideContainer()}>
// {this.renderLeftIcon()}
// {this.renderPrefix()}
// </span>
// <span className={styles.wrapper()}>
// {input}
// {this.renderPlaceholder()}
// </span>
// <span className={cx(styles.sideContainer(), styles.rightContainer())}>
// {this.renderSuffix()}
// {this.renderRightIcon()}
// </span>
// </label>
// );
Copy link
Member

Choose a reason for hiding this comment

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

Забыл удалить. Ниже тоже осталось.

@@ -69,7 +67,7 @@ export interface PagingProps extends CommonProps {
*
* @default false
*/
shouldBeVisibleWithLessThanTwoPages?: boolean;
shouldBeVisibleWithLessThanTwoPages?: boolean; // TODO Delete in 5.0
Copy link
Member

Choose a reason for hiding this comment

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

Лишняя тудушка)

Comment on lines 189 to 190
input2022(t: Theme) {
return css`
Copy link
Member

Choose a reason for hiding this comment

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

Неправильно слил стили.
В старом классе input() надо удалить все стили кроме первых 2-х правил.

position: absolute;
opacity: 0;

... далее стили из input2022

Comment on lines 250 to 255
return styles.activeHandleLarge(this.theme);
case 'medium':
return styles.activeHandleMedium(this.theme);
case 'small':
default:
return styles.activeHandleSmall(this.theme);
Copy link
Member

Choose a reason for hiding this comment

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

Эти классы остались в стилях.

@@ -1578,7 +1578,6 @@ export class DefaultTheme {
}

public static toggleBgFocus = 'linear-gradient(-180deg, #f1f1f1, #dedede)';
public static toggleBgActive = '#e5e5e5';
Copy link
Member

Choose a reason for hiding this comment

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

Вместо этой переменной теперь надо рекомендовать toggleContainerBgDisabled или toggleContainerBgDisabledChecked.
Вряд ли кодмод сейчас умеет заменять одну переменную на 2, но можно использовать первую.

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.

None yet

3 participants